From 826ab190c720218a0d207f00bdd641c7a5fdce7d Mon Sep 17 00:00:00 2001 From: shanforge <299346208+shanforge@users.noreply.github.com> Date: Fri, 4 Sep 2026 12:09:49 +0530 Subject: [PATCH 1/4] feat(mcp): adding, enabling, editing and removing a server in Settings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #83, and the last box of #63 — which closes #55. #76 landed everything about MCP that has no network in it: the server model, the namespace a server cannot pick, the trust rules, the four registry gates, the wire format and a transport. It was a draft for one reason — there was no way to *add* a server, so none of it could be reached. This is that way. ## The screen does not re-decide anything The rules live in `MCPServerStore`, `MCPEndpoint` and `MCPRegistryPlan` and were settled and tested before there was a screen. What this file is responsible for is not undoing them: adding does not enable, an edit moves the switch in neither direction, and the address is validated before it is saved. The field validates **while the user types**, and the message it shows is `MCPEndpoint.Rejection`'s rather than one written here, so the field and the store cannot disagree about what is allowed. It is deliberately not the only check — #76's review moved the rule into the store precisely because a rule that lives in a form is a rule the next caller does not get. ## The three loose ends #76's review recorded - **`MCPCatalog.refresh()` had no caller**, so `discovered` stayed empty and no MCP tool was ever published. It now runs when the section appears, when a server is switched on, and after an edit — a server switched on has no tool list yet, so without the second of those it would be enabled and offer nothing. - **`forget(id:)` had no caller**, so a removed server's tool list and health outlived it. Nothing offered them, because `MCPRegistryPlan` walks the store, but they were held for a server that no longer exists. - **`hasNetworkEgress` had no reader.** It is said once, plainly, next to the switches — rather than only on the Privacy tab, which is not where someone is standing when they turn a server on. ## Renaming a server no longer re-enables its tools The one this issue flagged as untested. A server's tools are published under a namespace derived from its **name**, and the per-tool disable list is keyed on the published name — the one the user saw when they turned the tool off. So renaming `GitHub` to `GitLab` turns `github__delete_repo` into `gitlab__delete_repo`, which is not on the list, and the tool comes back under a name the user has never seen. That is the worst direction for this kind of mistake to go: "I never want the agent to do this" has to survive an edit that had nothing to do with it, and there is no signal when it does not. `MCPRenameMigration` is the rule, pure and separate from both the store and the view — whatever else eventually renames a server has to apply it too, and a copy inside one form would not be. Where two servers fold to the same namespace the entry cannot be attributed to one of them and both move. That is the same ambiguity `MCPRegistryPlan` resolves by dropping the colliding tool, and it errs the same way: towards the tool staying **off**. Mutation-checked — making the remap a no-op turns "A disabled tool stays disabled through a rename" and "The rewritten name is one the registry will actually look for" red. The second is there because pasting the namespace and tool halves together can exceed the length bound `published` applies, and a name over that bound never matches anything. ## Verification - `xcodebuild build` — succeeds - `./scripts/test-no-llm.sh` — 1777 tests in 158 suites pass - SwiftFormat 0.62.1 `--lint` — 0/559 files require formatting - SwiftLint 0.65.0 `--strict` — 0 violations in 712 files Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01M3Wpnj9ZmWPKYdFPB1AVY3 --- Logue.xcodeproj/project.pbxproj | 12 + Logue/Agent/MCP/MCPRenameMigration.swift | 44 +++ Logue/Views/Settings/MCPServersSection.swift | 275 ++++++++++++++++++ Logue/Views/Settings/Tabs/AISettingsTab.swift | 5 + LogueTests/MCPRenameMigrationTests.swift | 73 +++++ 5 files changed, 409 insertions(+) create mode 100644 Logue/Agent/MCP/MCPRenameMigration.swift create mode 100644 Logue/Views/Settings/MCPServersSection.swift create mode 100644 LogueTests/MCPRenameMigrationTests.swift diff --git a/Logue.xcodeproj/project.pbxproj b/Logue.xcodeproj/project.pbxproj index a575458..23ba453 100644 --- a/Logue.xcodeproj/project.pbxproj +++ b/Logue.xcodeproj/project.pbxproj @@ -240,6 +240,7 @@ 4DFBBF9A94D043AEAE9573D1 /* SpaceFolderWriteTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 7F13A88F235D33A5674EC6CB /* SpaceFolderWriteTests.swift */; }; 4E544BEADC120DA66F931B07 /* MarkdownTextView.swift in Sources */ = {isa = PBXBuildFile; fileRef = 5CB6607FD58EE7074846C4DC /* MarkdownTextView.swift */; }; 4E5B9AA44205F22A4A113782 /* Suggestion.swift in Sources */ = {isa = PBXBuildFile; fileRef = F8106A5A4786A5595D00CBF2 /* Suggestion.swift */; }; + 4E71028643800BB98A439F98 /* MCPRenameMigration.swift in Sources */ = {isa = PBXBuildFile; fileRef = 8E5888F5EAA0144BB535EDF2 /* MCPRenameMigration.swift */; }; 4E90B4EE5AB06A18C7B15717 /* RichTextEditor.swift in Sources */ = {isa = PBXBuildFile; fileRef = F86FEEACED36C11C79D2FA9A /* RichTextEditor.swift */; }; 4E983F1CDE3D00085362D3EB /* DeepResearchCoordinator.swift in Sources */ = {isa = PBXBuildFile; fileRef = 00DB2AA9BA4A263B0524840D /* DeepResearchCoordinator.swift */; }; 4EC81490F679629FBCC10192 /* MeetingBreakdownCard.swift in Sources */ = {isa = PBXBuildFile; fileRef = 756E8155CF5415154F8B2C49 /* MeetingBreakdownCard.swift */; }; @@ -304,6 +305,7 @@ 606610B68EE510849B9358C3 /* MeetingWorkspaceView.swift in Sources */ = {isa = PBXBuildFile; fileRef = E6B117D634010EECE1C83553 /* MeetingWorkspaceView.swift */; }; 606932D135B847FCE58B9666 /* ShortcutRow.swift in Sources */ = {isa = PBXBuildFile; fileRef = 96FBFFF1C777A33CD084240C /* ShortcutRow.swift */; }; 60B8C522415BE5A837354AE4 /* InputDialogTools.swift in Sources */ = {isa = PBXBuildFile; fileRef = E2CEF58FA0555995987FD7AE /* InputDialogTools.swift */; }; + 613D7C325B26DC43F4CD0955 /* MCPRenameMigrationTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 64125C31BA8CDB08ADD703FA /* MCPRenameMigrationTests.swift */; }; 620F4FDE41C2DA66A77D420F /* TaskInspectorPanel.swift in Sources */ = {isa = PBXBuildFile; fileRef = 112B70D4C5B4DF51BEFCFB36 /* TaskInspectorPanel.swift */; }; 62C5D528384197C6CA17422E /* MenuBarIcon@2x.png in Resources */ = {isa = PBXBuildFile; fileRef = A0C424F1DB1CBE83620E9768 /* MenuBarIcon@2x.png */; }; 6373E69354AD314B2BA380F7 /* HomeCardShell.swift in Sources */ = {isa = PBXBuildFile; fileRef = 1E5007DA0D26B354ACEDDE7D /* HomeCardShell.swift */; }; @@ -435,6 +437,7 @@ 900D62D26E62CC1BD86F6152 /* AudioTimelineMixer.swift in Sources */ = {isa = PBXBuildFile; fileRef = 2CCEAA072E5CE2EDA0828E14 /* AudioTimelineMixer.swift */; }; 90CE4DBE3D79C429616DB00A /* AgentConversation.swift in Sources */ = {isa = PBXBuildFile; fileRef = 160A71994367176C72C11277 /* AgentConversation.swift */; }; 91504F26FF724A765B016AF2 /* whatsnew-wikilinks.png in Resources */ = {isa = PBXBuildFile; fileRef = F7E1089CE3E964A5BC0545A8 /* whatsnew-wikilinks.png */; }; + 9179F7B5B38B3E68AB712CC5 /* MCPServersSection.swift in Sources */ = {isa = PBXBuildFile; fileRef = 407A90F8E61C4E0576B95ADD /* MCPServersSection.swift */; }; 92251AAC9FFBCEA92E722619 /* AboutSettingsTab.swift in Sources */ = {isa = PBXBuildFile; fileRef = 4C618174E127839238F1ABD6 /* AboutSettingsTab.swift */; }; 924E8B180D4472C8BA2AA086 /* TranscriptGutter.swift in Sources */ = {isa = PBXBuildFile; fileRef = 0EBE94A9DFEBB0FEE09DD1F2 /* TranscriptGutter.swift */; }; 92ADFD50F966330CB6025480 /* PropertyValue.swift in Sources */ = {isa = PBXBuildFile; fileRef = 20FD91D8BED8E4CACE940391 /* PropertyValue.swift */; }; @@ -943,6 +946,7 @@ 402D8E4447C9B7063ADE7358 /* IslandThreadTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = IslandThreadTests.swift; sourceTree = ""; }; 40381D10206F56D057943756 /* SpaceContentPane.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = SpaceContentPane.swift; sourceTree = ""; }; 4066081DEC2821193FE7D882 /* WikiLinkURLTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = WikiLinkURLTests.swift; sourceTree = ""; }; + 407A90F8E61C4E0576B95ADD /* MCPServersSection.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = MCPServersSection.swift; sourceTree = ""; }; 417436A70F2C0F1DDA347DC4 /* AttachmentIntake.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = AttachmentIntake.swift; sourceTree = ""; }; 41C0416ED8CE675F6EF78FA0 /* TaskStoreTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = TaskStoreTests.swift; sourceTree = ""; }; 42BFEBF99163808EE4126279 /* HomeContextBar.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = HomeContextBar.swift; sourceTree = ""; }; @@ -1039,6 +1043,7 @@ 6405B6178237C69E2674D9E9 /* SystemAudioArmingTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = SystemAudioArmingTests.swift; sourceTree = ""; }; 640842B5780506F6C1A32F84 /* MarkdownStorageReenableTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = MarkdownStorageReenableTests.swift; sourceTree = ""; }; 6410A54BE5400DAAFADA2A88 /* SharedPolishComponents.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = SharedPolishComponents.swift; sourceTree = ""; }; + 64125C31BA8CDB08ADD703FA /* MCPRenameMigrationTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = MCPRenameMigrationTests.swift; sourceTree = ""; }; 6430E324276EBE1BD45A48BD /* PromptRegistry.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = PromptRegistry.swift; sourceTree = ""; }; 6445D0008F16B2DFE083F469 /* CommandCenterRecordingView.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = CommandCenterRecordingView.swift; sourceTree = ""; }; 64EBF5DF74CDD38CF0672831 /* SaveSummaryToDocumentSheet.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = SaveSummaryToDocumentSheet.swift; sourceTree = ""; }; @@ -1169,6 +1174,7 @@ 8DC85DAE7DF132DD4B355F61 /* PolishEngine.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = PolishEngine.swift; sourceTree = ""; }; 8DEC6591F262E1FB6400CA15 /* WritingNSTextView+SlashCommand.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = "WritingNSTextView+SlashCommand.swift"; sourceTree = ""; }; 8E366631BFF6453332DAB21D /* TaskTextParser.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = TaskTextParser.swift; sourceTree = ""; }; + 8E5888F5EAA0144BB535EDF2 /* MCPRenameMigration.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = MCPRenameMigration.swift; sourceTree = ""; }; 8E65B9D58A6CC515006C9A20 /* FileSystemTools.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = FileSystemTools.swift; sourceTree = ""; }; 8EF14F906DB3DB326245738F /* AgentConversationStore.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = AgentConversationStore.swift; sourceTree = ""; }; 8F3E9AD232A1D6AEB3064E31 /* DiarizationManager+BatchASR.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = "DiarizationManager+BatchASR.swift"; sourceTree = ""; }; @@ -1563,6 +1569,7 @@ 4856F91CAEDD15EA6FD63DE3 /* MCPHTTPTransport.swift */, 786248368D126D5970BA120F /* MCPRegistryPlan.swift */, B9815711174A0E38CAC49564 /* MCPRemoteTool.swift */, + 8E5888F5EAA0144BB535EDF2 /* MCPRenameMigration.swift */, 398663A67F5AC2B88579ECCE /* MCPServer.swift */, 22DDE4BB103EE762B9193C67 /* MCPServerHealth.swift */, 78FA8D2FFCB71F613943E7C5 /* MCPServerStore.swift */, @@ -1650,6 +1657,7 @@ 5650D423F2BE1586460EC7CD /* MCPInjectionTests.swift */, FA7C233F1363CFCDD985A945 /* MCPRegistryPlanTests.swift */, 83D720B453351470C9880F85 /* MCPRemoteToolTests.swift */, + 64125C31BA8CDB08ADD703FA /* MCPRenameMigrationTests.swift */, DE406D9272F3FF522E26024E /* MCPServerStoreTests.swift */, E13D8E299A128DA50DCB2D95 /* MCPWireFormatTests.swift */, 88A1A4DAACB05EBD1C285AF9 /* MessageActionsTests.swift */, @@ -1873,6 +1881,7 @@ ED83A7FADA1F871AEA2687DE /* BrowserExtensionSection.swift */, 344DDE65E3765B845016619D /* ExternalModelsSection.swift */, 3FF85EA0F9822CBD660E9AAB /* MarkdownStorageWarningSheet.swift */, + 407A90F8E61C4E0576B95ADD /* MCPServersSection.swift */, 38B7C47637B8FBFF7CC83C4F /* NetworkEgressSection.swift */, B57D98C9B0D1E6A69681C8C1 /* SettingsView.swift */, ); @@ -2902,6 +2911,7 @@ DC93BD43EA6E6F7471EDABFD /* MCPInjectionTests.swift in Sources */, B4BD1DFE81C73B14923168E1 /* MCPRegistryPlanTests.swift in Sources */, 957B3BD059A37354FC76CD38 /* MCPRemoteToolTests.swift in Sources */, + 613D7C325B26DC43F4CD0955 /* MCPRenameMigrationTests.swift in Sources */, 064E60464AEBFC09535CABB0 /* MCPServerStoreTests.swift in Sources */, 32F4E241050F113E21222AA8 /* MCPWireFormatTests.swift in Sources */, A138EE66F6C6808592F6F945 /* MarkdownDocumentFileTests.swift in Sources */, @@ -3250,9 +3260,11 @@ 4508AD94FB3EB3CF566D4FA9 /* MCPHTTPTransport.swift in Sources */, 94858FA8F83D9A3EA7A94BD0 /* MCPRegistryPlan.swift in Sources */, E85CACB47187DC6493AB23A0 /* MCPRemoteTool.swift in Sources */, + 4E71028643800BB98A439F98 /* MCPRenameMigration.swift in Sources */, FEDA0911BF52241492E9AA51 /* MCPServer.swift in Sources */, F596B64B4AD068E3645B5D70 /* MCPServerHealth.swift in Sources */, CDEE0252F385F4C1BC4EA128 /* MCPServerStore.swift in Sources */, + 9179F7B5B38B3E68AB712CC5 /* MCPServersSection.swift in Sources */, 6B6624468938CD6ADDA5781E /* MCPToolDescriptor.swift in Sources */, 43898343C424D0AAF75C9950 /* MCPToolNaming.swift in Sources */, 8749D7B4B33106238D10C12D /* MCPToolOutput.swift in Sources */, diff --git a/Logue/Agent/MCP/MCPRenameMigration.swift b/Logue/Agent/MCP/MCPRenameMigration.swift new file mode 100644 index 0000000..ac64b15 --- /dev/null +++ b/Logue/Agent/MCP/MCPRenameMigration.swift @@ -0,0 +1,44 @@ +import Foundation + +/// Keeping "never let the agent do X" true when the server providing X is renamed. +/// +/// A server's tools are published under a namespace derived from its **name** +/// (`MCPToolNaming`), and the per-tool disable list is keyed on the published name — the one +/// the user saw in Settings when they turned the tool off. Those two facts together mean a +/// rename silently re-enables everything: `github__delete_repo` becomes +/// `gitlab__delete_repo`, which is not on the list, so the tool is offered again. +/// +/// That is the worst direction for a mistake of this kind to go. "I never want the agent to +/// do this" has to survive an edit that had nothing to do with it, and the user gets no +/// signal — the tool simply becomes available again, under a name they have never seen. +/// +/// Pure, and separate from both the store and the view, because it is a rule rather than a +/// screen: whatever eventually renames a server has to apply it, and a copy inside one form +/// would not be applied by the next caller. +enum MCPRenameMigration { + /// The disable list, rewritten for a server that is changing name. + /// + /// Only entries under the old server's namespace move. Everything else — built-ins, and + /// other servers' tools — is returned untouched. + /// + /// - Note: if two servers fold to the same namespace (`GitHub` and `git hub` both give + /// `git_hub`), an entry cannot be attributed to one of them and the other's disabled + /// tools move too. That is the same ambiguity `MCPRegistryPlan` resolves by dropping + /// the colliding tool, and it errs the same way: towards the tool staying *off*, which + /// is the direction that cannot surprise anyone. + static func remapped(disabled: Set, from oldName: String, to newName: String) -> Set { + let oldNamespace = MCPToolNaming.namespace(for: oldName) + let newNamespace = MCPToolNaming.namespace(for: newName) + guard oldNamespace != newNamespace else { return disabled } + + return Set(disabled.map { entry in + guard let parts = MCPToolNaming.split(entry), parts.namespace == oldNamespace else { + return entry + } + // Rebuilt through `published` rather than by pasting the halves together, so the + // result obeys the same length bound as the name the registry will look for. A + // hand-assembled name that exceeds it would never match anything. + return MCPToolNaming.published(serverName: newName, toolName: parts.tool) + }) + } +} diff --git a/Logue/Views/Settings/MCPServersSection.swift b/Logue/Views/Settings/MCPServersSection.swift new file mode 100644 index 0000000..e3534bb --- /dev/null +++ b/Logue/Views/Settings/MCPServersSection.swift @@ -0,0 +1,275 @@ +import SwiftUI + +/// Adding, enabling, editing and removing the user's MCP servers. +/// +/// The rules this screen obeys are not its own — they live in `MCPServerStore`, +/// `MCPEndpoint` and `MCPRegistryPlan`, and were settled and tested before there was a +/// screen. What this file is responsible for is not *undoing* any of them: +/// +/// - **Adding is not enabling.** A new server arrives off. Turning it on is a separate, +/// deliberate act, because that is the act authorising network egress. +/// - **An edit changes neither direction of the switch.** Re-pointing a server is not consent +/// to start talking to the new address, and it is not a reason to stop talking to one the +/// user deliberately turned on. +/// - **The address is validated before it is saved**, and the field says why while the user is +/// still typing. `MCPEndpoint.validate` returns the message; this does not write its own. +/// It is *not* the only check — the store refuses a bad address too — and it must not become +/// so again, which is how the rule ended up enforced by nothing. +struct MCPServersSection: View { + @State private var store = MCPServerStore.shared + @State private var catalog = MCPCatalog.shared + + @AppStorage(AppConstants.UserDefaultsKeys.disabledAgentTools) + private var disabledToolsRaw: String = "" + + @State private var isAdding = false + @State private var editingID: UUID? + @State private var draftName = "" + @State private var draftAddress = "" + @State private var isRefreshing = false + + var body: some View { + VStack(alignment: .leading, spacing: 10) { + header + + if store.servers.isEmpty, !isAdding { + empty + } else { + ForEach(store.servers) { server in + row(for: server) + } + } + + if isAdding || editingID != nil { + form + } else { + Button("Add a server…") { beginAdding() } + .buttonStyle(.link) + .font(.callout) + } + } + .task { await refresh() } + } + + // MARK: - Header + + private var header: some View { + VStack(alignment: .leading, spacing: 2) { + HStack(spacing: 8) { + Text("MCP servers").font(.headline) + Spacer() + if isRefreshing { + ProgressView().controlSize(.small) + } + } + Text( + "Servers you add can give the agent extra tools. A server is off until you turn " + + "it on, and its tools are approved the same way Logue's own are." + ) + .font(.caption) + .foregroundStyle(.secondary) + + // Said once, plainly, where the switches are — rather than only on the Privacy + // tab, which is not where someone is when they turn one on. + if store.hasNetworkEgress { + Label( + "One or more enabled servers are not on this Mac. Their tools send data over the network.", + systemImage: "network" + ) + .font(.caption) + .foregroundStyle(.secondary) + .padding(.top, 2) + } + } + } + + private var empty: some View { + Text("No servers yet.") + .font(.callout) + .foregroundStyle(.secondary) + } + + // MARK: - A server + + @ViewBuilder + private func row(for server: MCPServer) -> some View { + if editingID == server.id { + EmptyView() + } else { + HStack(alignment: .firstTextBaseline, spacing: 10) { + Toggle( + isOn: Binding( + get: { server.isEnabled }, + set: { enable($0, for: server) } + ) + ) { + EmptyView() + } + .toggleStyle(.switch) + .labelsHidden() + .accessibilityLabel("\(server.name) enabled") + + VStack(alignment: .leading, spacing: 2) { + Text(server.name.isEmpty ? "Unnamed server" : server.name) + .font(.callout) + HStack(spacing: 6) { + // Shown to the user in full — it is their own address and they need to + // recognise it. It is never *logged*; that is the rule, and it is a + // different one. + Text(server.endpoint.absoluteString) + .font(.system(size: 10, design: .monospaced)) + .foregroundStyle(.tertiary) + .lineLimit(1) + .truncationMode(.middle) + if !MCPEndpoint.leavesTheMachine(server.endpoint) { + Text("on this Mac") + .font(.system(size: 10)) + .foregroundStyle(.tertiary) + } + } + Text(status(for: server).summary) + .font(.caption2) + .foregroundStyle(status(for: server).needsAttention ? AppThemeConstants.error : .secondary) + } + + Spacer() + + Button("Edit") { beginEditing(server) } + .buttonStyle(.link) + .font(.caption) + Button("Remove") { remove(server) } + .buttonStyle(.link) + .font(.caption) + .foregroundStyle(AppThemeConstants.error) + } + .padding(.vertical, 4) + } + } + + /// A server nobody has contacted has not failed — see `MCPServerHealth.State`. + private func status(for server: MCPServer) -> MCPServerHealth.State { + catalog.health[server.id] ?? .unknown + } + + // MARK: - The form + + private var form: some View { + VStack(alignment: .leading, spacing: 8) { + TextField("Name", text: $draftName) + .textFieldStyle(.roundedBorder) + TextField("https://example.com/mcp", text: $draftAddress) + .textFieldStyle(.roundedBorder) + + // While typing, not at save time. The message is `MCPEndpoint`'s so the field and + // the store cannot disagree about what is allowed. + if let rejection = liveRejection { + Text(rejection.message) + .font(.caption) + .foregroundStyle(AppThemeConstants.error) + } else if let url = validatedAddress, !MCPEndpoint.leavesTheMachine(url) { + Text("On this Mac, so nothing leaves it.") + .font(.caption) + .foregroundStyle(.secondary) + } + + HStack { + Button("Cancel") { cancel() } + Button(editingID == nil ? "Add" : "Save") { commit() } + .buttonStyle(.borderedProminent) + .disabled(!canCommit) + } + } + .padding(.vertical, 4) + } + + /// `nil` while the field is empty — an untouched field is not an error. + private var liveRejection: MCPEndpoint.Rejection? { + guard !draftAddress.trimmingCharacters(in: .whitespaces).isEmpty else { return nil } + if case let .failure(rejection) = MCPEndpoint.validate(draftAddress) { + return rejection + } + return nil + } + + private var validatedAddress: URL? { + if case let .success(url) = MCPEndpoint.validate(draftAddress) { + return url + } + return nil + } + + private var canCommit: Bool { + !draftName.trimmingCharacters(in: .whitespaces).isEmpty && validatedAddress != nil + } + + // MARK: - Doing things + + private func beginAdding() { + editingID = nil + draftName = "" + draftAddress = "" + isAdding = true + } + + private func beginEditing(_ server: MCPServer) { + isAdding = false + draftName = server.name + draftAddress = server.endpoint.absoluteString + editingID = server.id + } + + private func cancel() { + isAdding = false + editingID = nil + draftName = "" + draftAddress = "" + } + + private func commit() { + guard let url = validatedAddress else { return } + let name = draftName.trimmingCharacters(in: .whitespaces) + + if let editingID, let existing = store.servers.first(where: { $0.id == editingID }) { + // Before the store forgets the old name: what the user turned off is keyed on the + // published name, which is derived from it. See `MCPRenameMigration`. + migrateDisabledTools(from: existing.name, to: name) + store.update(id: editingID, name: name, endpoint: url) + } else { + store.add(name: name, endpoint: url) + } + cancel() + Task { await refresh() } + } + + private func remove(_ server: MCPServer) { + store.remove(id: server.id) + // Otherwise the server's last known tool list and health outlive it. Nothing offers + // them — `MCPRegistryPlan` walks the store — but they are held for a server that no + // longer exists, and would be read by anything that consults the catalog directly. + catalog.forget(id: server.id) + } + + private func enable(_ isEnabled: Bool, for server: MCPServer) { + store.setEnabled(isEnabled, for: server.id) + // A server that has just been switched on has no tool list yet, so without this it is + // enabled and offers nothing until something else happens to refresh. + Task { await refresh() } + } + + private func migrateDisabledTools(from oldName: String, to newName: String) { + let current = Set( + disabledToolsRaw.split(separator: ",") + .map { $0.trimmingCharacters(in: .whitespaces) } + .filter { !$0.isEmpty } + ) + let migrated = MCPRenameMigration.remapped(disabled: current, from: oldName, to: newName) + guard migrated != current else { return } + disabledToolsRaw = migrated.sorted().joined(separator: ",") + } + + private func refresh() async { + isRefreshing = true + await catalog.refresh() + isRefreshing = false + } +} diff --git a/Logue/Views/Settings/Tabs/AISettingsTab.swift b/Logue/Views/Settings/Tabs/AISettingsTab.swift index 65a8c4a..a5f07bb 100644 --- a/Logue/Views/Settings/Tabs/AISettingsTab.swift +++ b/Logue/Views/Settings/Tabs/AISettingsTab.swift @@ -16,6 +16,11 @@ struct AISettingsTab: View { Divider() toolsSection Divider() + // Right after the built-in tool list, because a server's tools join the same + // registry and obey the same per-tool switches — putting them in a different + // tab would suggest they are a different kind of thing. + MCPServersSection() + Divider() inferenceSection Divider() memorySection diff --git a/LogueTests/MCPRenameMigrationTests.swift b/LogueTests/MCPRenameMigrationTests.swift new file mode 100644 index 0000000..f40bacd --- /dev/null +++ b/LogueTests/MCPRenameMigrationTests.swift @@ -0,0 +1,73 @@ +import Foundation +import Testing +@testable import Logue + +/// Renaming a server must not undo what the user turned off. +@Suite("MCP rename migration") +struct MCPRenameMigrationTests { + @Test("A disabled tool stays disabled through a rename") + func disabledToolSurvivesRename() { + // The bug this exists for: published names are derived from the server name, and the + // disable list is keyed on the published name — so without this the tool is offered + // again under a name the user has never seen. + let before: Set = ["github__delete_repo"] + let after = MCPRenameMigration.remapped(disabled: before, from: "GitHub", to: "GitLab") + #expect(after.contains("gitlab__delete_repo")) + #expect(after.contains("github__delete_repo") == false) + } + + @Test("Built-in tools are never touched") + func builtInsAreUntouched() { + // A built-in has no namespace separator, so it cannot be mistaken for a server's. + let before: Set = ["delete_document", "run_javascript", "github__delete_repo"] + let after = MCPRenameMigration.remapped(disabled: before, from: "GitHub", to: "GitLab") + #expect(after.contains("delete_document")) + #expect(after.contains("run_javascript")) + } + + @Test("Another server's tools are left alone") + func otherServersAreUntouched() { + let before: Set = ["github__delete_repo", "jira__delete_issue"] + let after = MCPRenameMigration.remapped(disabled: before, from: "GitHub", to: "GitLab") + #expect(after.contains("jira__delete_issue")) + } + + @Test("A rename that does not change the namespace changes nothing") + func cosmeticRenameIsANoOp() { + // "GitHub" and "git-hub" both fold to `github`... they do not, and that is the point: + // the namespace is what matters, not the spelling. A change that leaves the namespace + // alone must leave the list byte-identical rather than rebuilding it. + let before: Set = ["github__delete_repo"] + #expect(MCPRenameMigration.remapped(disabled: before, from: "GitHub", to: "GITHUB") == before) + #expect(MCPRenameMigration.remapped(disabled: before, from: "GitHub", to: "GitHub") == before) + } + + @Test("Every entry survives the remap — nothing is silently dropped") + func nothingIsLost() { + let before: Set = [ + "delete_document", + "github__delete_repo", + "github__create_issue", + "jira__delete_issue", + ] + let after = MCPRenameMigration.remapped(disabled: before, from: "GitHub", to: "GitLab") + #expect(after.count == before.count, "an entry was lost or two collapsed into one") + } + + @Test("The rewritten name is one the registry will actually look for") + func rewrittenNameMatchesTheRegistry() { + // Pasting the halves together would let the result exceed the length bound + // `published` applies, and a name over that bound never matches anything. + let long = String(repeating: "server", count: 20) + let before: Set = [MCPToolNaming.published(serverName: "Short", toolName: "do_thing")] + let after = MCPRenameMigration.remapped(disabled: before, from: "Short", to: long) + let expected = MCPToolNaming.published(serverName: long, toolName: "do_thing") + #expect(after.contains(expected)) + #expect(expected.count <= MCPToolNaming.maxNameLength) + } + + @Test("An empty list stays empty") + func emptyStaysEmpty() { + #expect(MCPRenameMigration.remapped(disabled: [], from: "A", to: "B").isEmpty) + } +} From aebbe8aead128f3701ced4e370760d3a56b9d0d2 Mon Sep 17 00:00:00 2001 From: shanforge <299346208+shanforge@users.noreply.github.com> Date: Fri, 4 Sep 2026 12:15:48 +0530 Subject: [PATCH 2/4] fix: what the first review round found in the Settings screen MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **A refused save cleared the form and said nothing (major).** `commit()` called `store.add`/`store.update` and discarded the `Bool` they return, then closed the form and cleared the fields. The store validates the address independently — that was the point of moving the rule into it — so any disagreement between the field and the store meant pressing Add wiped what the user typed and saved nothing, with no message. The two agree today; this is what stops the day they don't being a silent loss. **A namespace collision was invisible (major).** Namespaces are derived from names and the derivation folds, so two servers can publish under one prefix — and `MCPRegistryPlan` resolves that by keeping the first and dropping the rest. Correct, and unobservable: the second server sits in Settings enabled, reachable, reporting its tool count, and offers the model nothing. It is now said while the name is still being typed. Not blocked — two servers may legitimately be called similar things — but named. **A decision was living inside a `View` (minor).** The clash check was a private computed property on the section, which is the shape `CLAUDE.md` singles out: a decision made inside a `View` is one the next caller cannot reach. `MCPNamespaceClash` is the rule, the view supplies the names. **Editing replaced the wrong thing (minor).** The row being edited returned `EmptyView()` and the form appeared below the whole list, so with several servers there was nothing saying which one was open. The form now replaces the row it is editing. ## A test that passed for the wrong reason, again The first version of the clash cases asserted that `git hub` clashes with `GitHub`. It does not: `GitHub` folds to `github` and `git hub` folds to `git_hub`, because a run of non-alphanumerics becomes a separator. They look like the same name and are not — and this is the *same pair* #76's review found already passing vacuously once, in the collision test there. There is now a case pinning that they do **not** clash, with a note saying why, so the next person to write this reaches for a genuinely-folding pair. Mutation-checked: comparing raw names instead of namespaces turns four cases red. Verified: build succeeds, 1785 tests in 159 suites pass, SwiftFormat --lint clean over 560 files, SwiftLint --strict 0 violations in 714 files. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01M3Wpnj9ZmWPKYdFPB1AVY3 --- Logue.xcodeproj/project.pbxproj | 8 + Logue/Agent/MCP/MCPNamespaceClash.swift | 30 ++++ Logue/Views/Settings/MCPServersSection.swift | 145 ++++++++++++------- LogueTests/MCPNamespaceClashTests.swift | 64 ++++++++ 4 files changed, 196 insertions(+), 51 deletions(-) create mode 100644 Logue/Agent/MCP/MCPNamespaceClash.swift create mode 100644 LogueTests/MCPNamespaceClashTests.swift diff --git a/Logue.xcodeproj/project.pbxproj b/Logue.xcodeproj/project.pbxproj index 23ba453..74d8a6d 100644 --- a/Logue.xcodeproj/project.pbxproj +++ b/Logue.xcodeproj/project.pbxproj @@ -326,6 +326,7 @@ 68523E427C1ECCA206B03149 /* ActionItemInboxPanel.swift in Sources */ = {isa = PBXBuildFile; fileRef = 5C89FFFB8EFE6EDE3560E782 /* ActionItemInboxPanel.swift */; }; 68A60F0021C2B996783F3F41 /* DocumentPropertyTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = FB0075D6BBFD557576AAF966 /* DocumentPropertyTests.swift */; }; 68E15F52C2575EED30F6D8AE /* TaskFileTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = B3B356E5C7937D4D7600108E /* TaskFileTests.swift */; }; + 691B28ABDBEEA10FB184EB2E /* MCPNamespaceClash.swift in Sources */ = {isa = PBXBuildFile; fileRef = 72F9D97B1C46C433A8908CB6 /* MCPNamespaceClash.swift */; }; 693843E5E8F7A5A45AAD99BA /* InlineDiagramView.swift in Sources */ = {isa = PBXBuildFile; fileRef = C2A85CBFCD11CDF2FB693F73 /* InlineDiagramView.swift */; }; 697553876307B2C83EA86595 /* ReviewModels.swift in Sources */ = {isa = PBXBuildFile; fileRef = DE44EB0DEEE58EA51F73402B /* ReviewModels.swift */; }; 69B2689283C249CBE8252ADE /* IslandSurfaceModifier.swift in Sources */ = {isa = PBXBuildFile; fileRef = 03E127468D8DFBEDBF111016 /* IslandSurfaceModifier.swift */; }; @@ -514,6 +515,7 @@ ACBD67F76F286A34F1A0E4CA /* DocumentWidthMode.swift in Sources */ = {isa = PBXBuildFile; fileRef = A62566632EE8FA6DAD6713DB /* DocumentWidthMode.swift */; }; ACF6D6515BF5CB9F03B85496 /* QuickOpenPaletteView.swift in Sources */ = {isa = PBXBuildFile; fileRef = CABE8A04DD20F09B12BEDB15 /* QuickOpenPaletteView.swift */; }; AD07D0DCE8536510D087A05C /* TroubleshootingActions.swift in Sources */ = {isa = PBXBuildFile; fileRef = 15DD5F0F004960A0D0B66E8B /* TroubleshootingActions.swift */; }; + AE20DB2F67CD986E3DEEAC19 /* MCPNamespaceClashTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = F475CCA42BE819BF19DD4FA4 /* MCPNamespaceClashTests.swift */; }; AE7BCB76D77404A8B71D119E /* PIIModels.swift in Sources */ = {isa = PBXBuildFile; fileRef = 3B9978579DD7A8D5743C9DB6 /* PIIModels.swift */; }; AF35E4BCA1244DFE4B983329 /* UndoToastView.swift in Sources */ = {isa = PBXBuildFile; fileRef = AFBC89D908FFD586D1C38ABF /* UndoToastView.swift */; }; AFCA039A58319E9F99A6A763 /* VerifyModels.swift in Sources */ = {isa = PBXBuildFile; fileRef = 4F31BC63173139D2C8BB0D1D /* VerifyModels.swift */; }; @@ -1086,6 +1088,7 @@ 71CCC37048141456C9BA5AA3 /* DocumentSearchBar.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = DocumentSearchBar.swift; sourceTree = ""; }; 7250A0C394AFABC041C58D68 /* DeepResearchCoordinator+Helpers.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = "DeepResearchCoordinator+Helpers.swift"; sourceTree = ""; }; 72EE330160AA0276B2907F2A /* MeetingRowCompact.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = MeetingRowCompact.swift; sourceTree = ""; }; + 72F9D97B1C46C433A8908CB6 /* MCPNamespaceClash.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = MCPNamespaceClash.swift; sourceTree = ""; }; 7390A75F79EE786998273DA7 /* IslandMotion.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = IslandMotion.swift; sourceTree = ""; }; 739A71E158624FCF8E71C075 /* PostRecordingPipeline.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = PostRecordingPipeline.swift; sourceTree = ""; }; 73A0577F01A5B33292317D52 /* SummaryNarrationService.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = SummaryNarrationService.swift; sourceTree = ""; }; @@ -1467,6 +1470,7 @@ F26F4F42013205EAE3A14D9D /* HomeContinueGrid.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = HomeContinueGrid.swift; sourceTree = ""; }; F30E821E71C80A341EEC3F68 /* BlockEditorView+Reorder.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = "BlockEditorView+Reorder.swift"; sourceTree = ""; }; F3BC206B7D135B2017B99C1D /* LinkIndexTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = LinkIndexTests.swift; sourceTree = ""; }; + F475CCA42BE819BF19DD4FA4 /* MCPNamespaceClashTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = MCPNamespaceClashTests.swift; sourceTree = ""; }; F4837D7E60145F15B37A2310 /* GraphRetriever.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = GraphRetriever.swift; sourceTree = ""; }; F4D0B06CDDC1184FD7958F9A /* SuggestionCardView.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = SuggestionCardView.swift; sourceTree = ""; }; F520732BF6E86EA62DAC3E2B /* AgentTool.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = AgentTool.swift; sourceTree = ""; }; @@ -1567,6 +1571,7 @@ children = ( 7A0EC77C2B3E1C387145AC15 /* MCPCatalog.swift */, 4856F91CAEDD15EA6FD63DE3 /* MCPHTTPTransport.swift */, + 72F9D97B1C46C433A8908CB6 /* MCPNamespaceClash.swift */, 786248368D126D5970BA120F /* MCPRegistryPlan.swift */, B9815711174A0E38CAC49564 /* MCPRemoteTool.swift */, 8E5888F5EAA0144BB535EDF2 /* MCPRenameMigration.swift */, @@ -1655,6 +1660,7 @@ 640842B5780506F6C1A32F84 /* MarkdownStorageReenableTests.swift */, 216D7BAD368F51D5920D02E5 /* MCPFoundationsTests.swift */, 5650D423F2BE1586460EC7CD /* MCPInjectionTests.swift */, + F475CCA42BE819BF19DD4FA4 /* MCPNamespaceClashTests.swift */, FA7C233F1363CFCDD985A945 /* MCPRegistryPlanTests.swift */, 83D720B453351470C9880F85 /* MCPRemoteToolTests.swift */, 64125C31BA8CDB08ADD703FA /* MCPRenameMigrationTests.swift */, @@ -2909,6 +2915,7 @@ C8E1AB4ED265C0B97465C5BD /* LongRecordingE2ETests.swift in Sources */, 6810260973C01161B9183B44 /* MCPFoundationsTests.swift in Sources */, DC93BD43EA6E6F7471EDABFD /* MCPInjectionTests.swift in Sources */, + AE20DB2F67CD986E3DEEAC19 /* MCPNamespaceClashTests.swift in Sources */, B4BD1DFE81C73B14923168E1 /* MCPRegistryPlanTests.swift in Sources */, 957B3BD059A37354FC76CD38 /* MCPRemoteToolTests.swift in Sources */, 613D7C325B26DC43F4CD0955 /* MCPRenameMigrationTests.swift in Sources */, @@ -3258,6 +3265,7 @@ F03521AB1AD681F4D08060C3 /* LogueApp.swift in Sources */, 2944C84805092418E45892EA /* MCPCatalog.swift in Sources */, 4508AD94FB3EB3CF566D4FA9 /* MCPHTTPTransport.swift in Sources */, + 691B28ABDBEEA10FB184EB2E /* MCPNamespaceClash.swift in Sources */, 94858FA8F83D9A3EA7A94BD0 /* MCPRegistryPlan.swift in Sources */, E85CACB47187DC6493AB23A0 /* MCPRemoteTool.swift in Sources */, 4E71028643800BB98A439F98 /* MCPRenameMigration.swift in Sources */, diff --git a/Logue/Agent/MCP/MCPNamespaceClash.swift b/Logue/Agent/MCP/MCPNamespaceClash.swift new file mode 100644 index 0000000..8461bf3 --- /dev/null +++ b/Logue/Agent/MCP/MCPNamespaceClash.swift @@ -0,0 +1,30 @@ +import Foundation + +/// Whether a server's name would publish its tools under a prefix another server already has. +/// +/// Namespaces are *derived* from names — that is what stops a server picking one that +/// shadows a built-in — and the derivation folds: `GitHub`, `git hub` and `GIT-HUB` all give +/// `git_hub`. Two servers landing on the same namespace publish the same names, and +/// `MCPRegistryPlan` resolves that by keeping the first and dropping the rest. +/// +/// Which is the right resolution, and an invisible one. The second server sits in Settings +/// enabled, reachable, reporting its tool count — and offers the model nothing, with nothing +/// on screen saying why. So the clash is worth naming *before* it is created, which means +/// answering it while the user is still typing. +/// +/// Pure, and outside the view, because a decision made inside a `View` is one the next caller +/// cannot reach — the same reason `AskRouter` is a function rather than a branch in a body. +enum MCPNamespaceClash { + /// The first of `existing` that folds to the same namespace as `candidate`. + /// + /// - Parameters: + /// - candidate: the name being typed. + /// - existing: the names of the other servers — the caller excludes the one being + /// edited, because a server does not clash with itself. + static func first(for candidate: String, among existing: [String]) -> String? { + let trimmed = candidate.trimmingCharacters(in: .whitespaces) + guard !trimmed.isEmpty else { return nil } + let namespace = MCPToolNaming.namespace(for: trimmed) + return existing.first { MCPToolNaming.namespace(for: $0) == namespace } + } +} diff --git a/Logue/Views/Settings/MCPServersSection.swift b/Logue/Views/Settings/MCPServersSection.swift index e3534bb..7486a90 100644 --- a/Logue/Views/Settings/MCPServersSection.swift +++ b/Logue/Views/Settings/MCPServersSection.swift @@ -27,6 +27,7 @@ struct MCPServersSection: View { @State private var draftName = "" @State private var draftAddress = "" @State private var isRefreshing = false + @State private var saveFailed = false var body: some View { VStack(alignment: .leading, spacing: 10) { @@ -36,13 +37,20 @@ struct MCPServersSection: View { empty } else { ForEach(store.servers) { server in - row(for: server) + // The form replaces the row it is editing, rather than the row vanishing + // and a form appearing under the whole list — with several servers, that + // leaves no way to tell which one is being edited. + if editingID == server.id { + form + } else { + row(for: server) + } } } - if isAdding || editingID != nil { + if isAdding { form - } else { + } else if editingID == nil { Button("Add a server…") { beginAdding() } .buttonStyle(.link) .font(.callout) @@ -91,59 +99,54 @@ struct MCPServersSection: View { // MARK: - A server - @ViewBuilder private func row(for server: MCPServer) -> some View { - if editingID == server.id { - EmptyView() - } else { - HStack(alignment: .firstTextBaseline, spacing: 10) { - Toggle( - isOn: Binding( - get: { server.isEnabled }, - set: { enable($0, for: server) } - ) - ) { - EmptyView() - } - .toggleStyle(.switch) - .labelsHidden() - .accessibilityLabel("\(server.name) enabled") - - VStack(alignment: .leading, spacing: 2) { - Text(server.name.isEmpty ? "Unnamed server" : server.name) - .font(.callout) - HStack(spacing: 6) { - // Shown to the user in full — it is their own address and they need to - // recognise it. It is never *logged*; that is the rule, and it is a - // different one. - Text(server.endpoint.absoluteString) - .font(.system(size: 10, design: .monospaced)) + HStack(alignment: .firstTextBaseline, spacing: 10) { + Toggle( + isOn: Binding( + get: { server.isEnabled }, + set: { enable($0, for: server) } + ) + ) { + EmptyView() + } + .toggleStyle(.switch) + .labelsHidden() + .accessibilityLabel("\(server.name) enabled") + + VStack(alignment: .leading, spacing: 2) { + Text(server.name.isEmpty ? "Unnamed server" : server.name) + .font(.callout) + HStack(spacing: 6) { + // Shown to the user in full — it is their own address and they need to + // recognise it. It is never *logged*; that is the rule, and it is a + // different one. + Text(server.endpoint.absoluteString) + .font(.system(size: 10, design: .monospaced)) + .foregroundStyle(.tertiary) + .lineLimit(1) + .truncationMode(.middle) + if !MCPEndpoint.leavesTheMachine(server.endpoint) { + Text("on this Mac") + .font(.system(size: 10)) .foregroundStyle(.tertiary) - .lineLimit(1) - .truncationMode(.middle) - if !MCPEndpoint.leavesTheMachine(server.endpoint) { - Text("on this Mac") - .font(.system(size: 10)) - .foregroundStyle(.tertiary) - } } - Text(status(for: server).summary) - .font(.caption2) - .foregroundStyle(status(for: server).needsAttention ? AppThemeConstants.error : .secondary) } + Text(status(for: server).summary) + .font(.caption2) + .foregroundStyle(status(for: server).needsAttention ? AppThemeConstants.error : .secondary) + } - Spacer() + Spacer() - Button("Edit") { beginEditing(server) } - .buttonStyle(.link) - .font(.caption) - Button("Remove") { remove(server) } - .buttonStyle(.link) - .font(.caption) - .foregroundStyle(AppThemeConstants.error) - } - .padding(.vertical, 4) + Button("Edit") { beginEditing(server) } + .buttonStyle(.link) + .font(.caption) + Button("Remove") { remove(server) } + .buttonStyle(.link) + .font(.caption) + .foregroundStyle(AppThemeConstants.error) } + .padding(.vertical, 4) } /// A server nobody has contacted has not failed — see `MCPServerHealth.State`. @@ -172,6 +175,21 @@ struct MCPServersSection: View { .foregroundStyle(.secondary) } + if let clash = namespaceClash { + Text( + "“\(clash)” already publishes its tools under the same prefix. " + + "Whichever comes first keeps the names; the other's tools are dropped." + ) + .font(.caption) + .foregroundStyle(.secondary) + } + + if saveFailed { + Text("That server could not be saved. Check the address.") + .font(.caption) + .foregroundStyle(AppThemeConstants.error) + } + HStack { Button("Cancel") { cancel() } Button(editingID == nil ? "Add" : "Save") { commit() } @@ -202,6 +220,19 @@ struct MCPServersSection: View { !draftName.trimmingCharacters(in: .whitespaces).isEmpty && validatedAddress != nil } + /// Another server whose name folds to the same namespace as the one being typed. + /// + /// Not blocked — two servers may legitimately be called similar things — but said, because + /// the consequence is otherwise invisible. The rule is `MCPNamespaceClash`'s; this only + /// supplies the names, and excludes the server being edited, which cannot clash with + /// itself. + private var namespaceClash: String? { + MCPNamespaceClash.first( + for: draftName, + among: store.servers.filter { $0.id != editingID }.map(\.name) + ) + } + // MARK: - Doing things private func beginAdding() { @@ -223,19 +254,31 @@ struct MCPServersSection: View { editingID = nil draftName = "" draftAddress = "" + saveFailed = false } private func commit() { guard let url = validatedAddress else { return } let name = draftName.trimmingCharacters(in: .whitespaces) + // The store validates too, and its answer is the one that decides. Taking it rather + // than discarding it is the difference between a refusal the user can see and a + // button that closes the form, clears the fields and saves nothing — the two + // validators agree today, and the day one of them gains a rule the other has not, + // this is what stops that becoming a silent loss of what they typed. + var saved = false if let editingID, let existing = store.servers.first(where: { $0.id == editingID }) { // Before the store forgets the old name: what the user turned off is keyed on the // published name, which is derived from it. See `MCPRenameMigration`. migrateDisabledTools(from: existing.name, to: name) - store.update(id: editingID, name: name, endpoint: url) + saved = store.update(id: editingID, name: name, endpoint: url) } else { - store.add(name: name, endpoint: url) + saved = store.add(name: name, endpoint: url) + } + + guard saved else { + saveFailed = true + return } cancel() Task { await refresh() } diff --git a/LogueTests/MCPNamespaceClashTests.swift b/LogueTests/MCPNamespaceClashTests.swift new file mode 100644 index 0000000..bb3f4e5 --- /dev/null +++ b/LogueTests/MCPNamespaceClashTests.swift @@ -0,0 +1,64 @@ +import Testing +@testable import Logue + +/// Two servers publishing under one prefix is resolved by dropping tools. Saying so before +/// it happens is the only way the user finds out. +@Suite("MCP namespace clash") +struct MCPNamespaceClashTests { + @Test("Names that fold to the same namespace clash") + func foldedNamesClash() { + // Every one of these folds to `git_hub`: the derivation lowercases and turns each run + // of non-alphanumerics into a single underscore. None of them look alike at a glance, + // which is the whole reason the clash has to be said out loud. + for name in ["GIT-HUB", "Git.Hub", "git__hub", "git hub"] { + #expect( + MCPNamespaceClash.first(for: name, among: ["git hub"]) == "git hub", + "\(name) should clash with 'git hub'" + ) + } + } + + @Test("Case and spelling alone are a clash") + func caseOnlyNamesClash() { + #expect(MCPNamespaceClash.first(for: "GITHUB", among: ["GitHub"]) == "GitHub") + } + + @Test("A separator is what tells two names apart, and it is easy to misread") + func separatorMakesTheDifference() { + // `GitHub` folds to `github`; `git hub` folds to `git_hub`. They look like the same + // name and are not, and asserting that they clash is a case that passes for the wrong + // reason — it is the exact pair #76's review found already doing that once. + #expect(MCPNamespaceClash.first(for: "git hub", among: ["GitHub"]) == nil) + #expect(MCPToolNaming.namespace(for: "GitHub") != MCPToolNaming.namespace(for: "git hub")) + } + + @Test("Genuinely different names do not clash") + func differentNamesDoNotClash() { + #expect(MCPNamespaceClash.first(for: "Jira", among: ["GitHub", "Linear"]) == nil) + } + + @Test("An empty or blank name clashes with nothing") + func blankNameIsNotAClash() { + #expect(MCPNamespaceClash.first(for: "", among: ["GitHub"]) == nil) + #expect(MCPNamespaceClash.first(for: " ", among: ["GitHub"]) == nil) + } + + @Test("Nothing to clash with is not a clash") + func noServersIsNotAClash() { + #expect(MCPNamespaceClash.first(for: "GitHub", among: []) == nil) + } + + @Test("The clash it reports is the one the registry would resolve first") + func reportsTheFirstClash() { + // `MCPRegistryPlan` keeps the first claim over an ordered list, so the name worth + // showing is the one that would win — not merely any of them. + #expect(MCPNamespaceClash.first(for: "Git.Hub", among: ["git hub", "GIT-HUB"]) == "git hub") + } + + @Test("A name of nothing but punctuation clashes with another of the same") + func punctuationNamesShareTheFallback() { + // Both fold to the `server` fallback, so they really do collide — reporting them as + // distinct would be the wrong answer, not a kinder one. + #expect(MCPNamespaceClash.first(for: "!!!", among: ["???"]) == "???") + } +} From 69fe5c15af3a223233a804ed902165858e55eb1b Mon Sep 17 00:00:00 2001 From: shanforge <299346208+shanforge@users.noreply.github.com> Date: Fri, 4 Sep 2026 12:18:47 +0530 Subject: [PATCH 3/4] fix: what the second review round found MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **The spinner stopped before the work did (minor).** `isRefreshing` was a boolean, so enabling two servers in quick succession started two refreshes and the first to finish cleared it — leaving the row that had not been contacted yet looking settled. It is a count of what is in flight now, cleared in a `defer` so a cancelled task cannot leave it above zero and the spinner turning for the rest of the session. **A removed server could come back (minor).** `MCPCatalog.refresh()` read the server list when it built its task group and never again. A refresh takes as long as its slowest server, and the user can remove one while it runs: `forget(id:)` cleared the entries and the results loop then wrote them straight back, for a server that no longer exists. Nothing would have offered those tools — `MCPRegistryPlan` walks the store — but they would be held for the rest of the session, and anything reading the catalog directly would have seen them. The list is re-read as each result arrives. Mutation-checked: dropping the guard turns "A server removed mid-refresh does not come back" red, driven through a deliberately slow stub transport so no test touches a socket. Verified: build succeeds, 1786 tests in 160 suites pass, SwiftFormat --lint clean over 560 files, SwiftLint --strict 0 violations in 714 files. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01M3Wpnj9ZmWPKYdFPB1AVY3 --- Logue/Agent/MCP/MCPCatalog.swift | 8 ++++ Logue/Views/Settings/MCPServersSection.swift | 16 ++++++-- LogueTests/MCPRenameMigrationTests.swift | 41 ++++++++++++++++++++ 3 files changed, 61 insertions(+), 4 deletions(-) diff --git a/Logue/Agent/MCP/MCPCatalog.swift b/Logue/Agent/MCP/MCPCatalog.swift index 76535e9..6f4780b 100644 --- a/Logue/Agent/MCP/MCPCatalog.swift +++ b/Logue/Agent/MCP/MCPCatalog.swift @@ -65,6 +65,14 @@ final class MCPCatalog { } } for await (id, result) in group { + // The server list is read again here, not only when the group was built. A + // refresh takes as long as the slowest server, and the user can remove one + // while it runs — `forget(id:)` would clear its entries and this loop would + // then write them straight back, for a server that no longer exists. Nothing + // would offer those tools, because `MCPRegistryPlan` walks the store, but + // they would be held for the rest of the session. + guard store.servers.contains(where: { $0.id == id }) else { continue } + switch result { case let .success(descriptors): discovered[id] = descriptors diff --git a/Logue/Views/Settings/MCPServersSection.swift b/Logue/Views/Settings/MCPServersSection.swift index 7486a90..fcc647b 100644 --- a/Logue/Views/Settings/MCPServersSection.swift +++ b/Logue/Views/Settings/MCPServersSection.swift @@ -26,7 +26,12 @@ struct MCPServersSection: View { @State private var editingID: UUID? @State private var draftName = "" @State private var draftAddress = "" - @State private var isRefreshing = false + /// How many refreshes are in flight, rather than whether one is. + /// + /// Enabling two servers in quick succession starts two, and with a boolean the first to + /// finish clears the spinner while the second is still going — so the row that has not + /// been contacted yet looks settled. A count only reaches zero when the last one ends. + @State private var refreshesInFlight = 0 @State private var saveFailed = false var body: some View { @@ -66,7 +71,7 @@ struct MCPServersSection: View { HStack(spacing: 8) { Text("MCP servers").font(.headline) Spacer() - if isRefreshing { + if refreshesInFlight > 0 { ProgressView().controlSize(.small) } } @@ -311,8 +316,11 @@ struct MCPServersSection: View { } private func refresh() async { - isRefreshing = true + refreshesInFlight += 1 + // `defer` rather than a line after the await: the task is cancelled when this section + // goes away mid-refresh, and a counter left above zero by a cancellation would leave + // the spinner turning for the rest of the session. + defer { refreshesInFlight -= 1 } await catalog.refresh() - isRefreshing = false } } diff --git a/LogueTests/MCPRenameMigrationTests.swift b/LogueTests/MCPRenameMigrationTests.swift index f40bacd..fe67405 100644 --- a/LogueTests/MCPRenameMigrationTests.swift +++ b/LogueTests/MCPRenameMigrationTests.swift @@ -71,3 +71,44 @@ struct MCPRenameMigrationTests { #expect(MCPRenameMigration.remapped(disabled: [], from: "A", to: "B").isEmpty) } } + +/// Removing a server while it is being contacted must not resurrect it. +@Suite("MCP catalog lifecycle") +@MainActor +struct MCPCatalogLifecycleTests { + private struct SlowTransport: MCPTransport { + func listTools(server _: MCPServer) async throws -> [MCPToolDescriptor] { + // Long enough that the removal below lands first. + try await Task.sleep(for: .milliseconds(120)) + return [MCPToolDescriptor(name: "do_thing", description: "")] + } + + func call(server _: MCPServer, tool _: String, arguments _: [String: Any]) async throws -> String { "" } + } + + @Test("A server removed mid-refresh does not come back") + func removedServerIsNotResurrected() async throws { + let suiteName = "mcp.catalog.lifecycle" + let defaults = try #require(UserDefaults(suiteName: suiteName)) + defaults.removePersistentDomain(forName: suiteName) + + let store = MCPServerStore(defaults: defaults, key: "mcp.catalog.lifecycle.servers") + let url = try #require(URL(string: "https://mcp.example.com/rpc")) + #expect(store.add(name: "Doomed", endpoint: url)) + let id = try #require(store.servers.first?.id) + store.setEnabled(true, for: id) + + let catalog = MCPCatalog(store: store, transport: SlowTransport()) + async let running: Void = catalog.refresh() + + // Remove it while the transport is still thinking. + try await Task.sleep(for: .milliseconds(30)) + store.remove(id: id) + catalog.forget(id: id) + + await running + + #expect(catalog.discovered[id] == nil, "the removed server's tool list came back") + #expect(catalog.health[id] == nil, "the removed server's health came back") + } +} From 5cd4694dc6902839012c00da4c02f09d1d292ecf Mon Sep 17 00:00:00 2001 From: shanforge <299346208+shanforge@users.noreply.github.com> Date: Fri, 4 Sep 2026 12:20:15 +0530 Subject: [PATCH 4/4] fix: name the row a control belongs to MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round three. Every server's Edit and Remove announced only "Edit" and "Remove", so with several servers a VoiceOver user was told which action but not which server — and Remove is the one where guessing is expensive. `displayName(of:)` also gives the empty-name case one answer rather than two. A name can be empty: the decoder defaults it so a file written before the field existed still reads, and a row labelled with nothing cannot be told from the next one, on screen or aloud. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01M3Wpnj9ZmWPKYdFPB1AVY3 --- Logue/Views/Settings/MCPServersSection.swift | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/Logue/Views/Settings/MCPServersSection.swift b/Logue/Views/Settings/MCPServersSection.swift index fcc647b..1917bfd 100644 --- a/Logue/Views/Settings/MCPServersSection.swift +++ b/Logue/Views/Settings/MCPServersSection.swift @@ -119,7 +119,7 @@ struct MCPServersSection: View { .accessibilityLabel("\(server.name) enabled") VStack(alignment: .leading, spacing: 2) { - Text(server.name.isEmpty ? "Unnamed server" : server.name) + Text(displayName(of: server)) .font(.callout) HStack(spacing: 6) { // Shown to the user in full — it is their own address and they need to @@ -143,17 +143,31 @@ struct MCPServersSection: View { Spacer() + // Named for the row they belong to. With several servers, a list of buttons all + // announcing "Edit" tells a VoiceOver user which action but not which server — + // and Remove is the one where guessing is expensive. Button("Edit") { beginEditing(server) } .buttonStyle(.link) .font(.caption) + .accessibilityLabel("Edit \(displayName(of: server))") Button("Remove") { remove(server) } .buttonStyle(.link) .font(.caption) .foregroundStyle(AppThemeConstants.error) + .accessibilityLabel("Remove \(displayName(of: server))") } .padding(.vertical, 4) } + /// What to call a server on screen. + /// + /// A name can be empty — the decoder defaults it to `""` so a file written before the + /// field existed still reads — and a row labelled with nothing is a row that cannot be + /// told apart from the next one, in the list and in VoiceOver alike. + private func displayName(of server: MCPServer) -> String { + server.name.isEmpty ? "Unnamed server" : server.name + } + /// A server nobody has contacted has not failed — see `MCPServerHealth.State`. private func status(for server: MCPServer) -> MCPServerHealth.State { catalog.health[server.id] ?? .unknown