From a91552e49a9f2585ffc6abfdee2621ea6d9420da Mon Sep 17 00:00:00 2001 From: credence-the-bot Date: Tue, 12 May 2026 18:41:21 +0000 Subject: [PATCH] fix(macos): surface duplicate-name error on provider connection create MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Marina QA flag (Slack 2026-05-12): creating a new provider connection with a duplicate name showed the generic "Couldn't create connection. Please try again." instead of telling the user the name is taken. Daemon returns a proper 409 ConflictError with a structured envelope, but the shared `createProviderConnection` returned `Optional` so the status code (and the user-actionable reason) was being thrown away. This PR mirrors the existing `ProviderConnectionDeleteResult` pattern: * Introduce `ProviderConnectionCreateResult` with cases `.created/.duplicate/.invalid(message:)/.error`. * Switch on `response.statusCode` in `ProviderConnectionClient.create`: 200-299 → `.created`, 409 → `.duplicate`, 400 → `.invalid` (carrying the daemon's `error.message` from the standard envelope), otherwise `.error`. * Update `ProvidersSheet.commitEditor` to switch on the result and surface a precise message per branch — matches the web modal's existing wording (`A connection named "X" already exists.`) for cross-client parity. * Update the mock + happy-path / duplicate / invalid / error tests to the new enum surface. Web (`provider-editor-modal.tsx`) already handles 409 correctly; this is macOS-only catch-up. No daemon changes. --- .../Features/Settings/ProvidersSheet.swift | 25 +++++++-- .../Settings/ProvidersSheetTests.swift | 4 +- .../ProviderConnectionClientTests.swift | 47 ++++++++++++---- .../Network/ProviderConnectionClient.swift | 53 +++++++++++++++++-- 4 files changed, 108 insertions(+), 21 deletions(-) diff --git a/clients/macos/vellum-assistant/Features/Settings/ProvidersSheet.swift b/clients/macos/vellum-assistant/Features/Settings/ProvidersSheet.swift index 72286929365..6121c2402f8 100644 --- a/clients/macos/vellum-assistant/Features/Settings/ProvidersSheet.swift +++ b/clients/macos/vellum-assistant/Features/Settings/ProvidersSheet.swift @@ -987,18 +987,35 @@ struct ProvidersSheet: View { switch editorState { case .create: - guard let created = await client.createProviderConnection( + let result = await client.createProviderConnection( name: name, provider: draft.provider, auth: auth, label: label, status: status - ) else { + ) + switch result { + case .created(let created): + connections.append(created) + editorState = nil + case .duplicate: + // 409 — surface the actual reason instead of the generic + // fallback so users immediately see that the key (the + // editor's "Key" field, which becomes the connection's + // unique name) collides with an existing connection. + actionError = "A connection named \"\(name)\" already exists." + return + case .invalid(let message): + // 400 — daemon often includes a useful structured reason + // (e.g. `Invalid provider "x". Valid: ...`). Surface it + // verbatim when present; fall back to a generic invalid + // message otherwise. + actionError = message ?? "Invalid configuration. Check the provider and auth settings." + return + case .error: actionError = "Couldn't create connection. Please try again." return } - connections.append(created) - editorState = nil case .edit(let originalName), .managedEdit(let originalName): // Managed-edit and user-edit share the same PATCH path. The auth diff --git a/clients/macos/vellum-assistantTests/Features/Settings/ProvidersSheetTests.swift b/clients/macos/vellum-assistantTests/Features/Settings/ProvidersSheetTests.swift index b5791485af1..d34ff275a47 100644 --- a/clients/macos/vellum-assistantTests/Features/Settings/ProvidersSheetTests.swift +++ b/clients/macos/vellum-assistantTests/Features/Settings/ProvidersSheetTests.swift @@ -78,7 +78,7 @@ final class ProvidersSheetTests: XCTestCase { func testCreateCallsClientWithExpectedArguments() async { let created = makeConnection(name: "new-conn", provider: "openai", authType: "api_key") - mockClient.createResponse = created + mockClient.createResponse = .created(created) _ = await mockClient.createProviderConnection( name: "new-conn", @@ -244,7 +244,7 @@ final class ProvidersSheetTests: XCTestCase { authType: "api_key", label: "Anthropic" ) - mockClient.createResponse = forked + mockClient.createResponse = .created(forked) _ = await mockClient.createProviderConnection( name: "anthropic-personal", diff --git a/clients/macos/vellum-assistantTests/Network/ProviderConnectionClientTests.swift b/clients/macos/vellum-assistantTests/Network/ProviderConnectionClientTests.swift index 8a130cfafd2..0c210056689 100644 --- a/clients/macos/vellum-assistantTests/Network/ProviderConnectionClientTests.swift +++ b/clients/macos/vellum-assistantTests/Network/ProviderConnectionClientTests.swift @@ -38,9 +38,9 @@ final class MockProviderConnectionClient: ProviderConnectionClientProtocol { var createAuthArg: ProviderConnectionAuth? var createLabelArg: String? var createStatusArg: ConnectionStatus? - var createResponse: ProviderConnection? = nil + var createResponse: ProviderConnectionCreateResult = .error - func createProviderConnection(name: String, provider: String, auth: ProviderConnectionAuth, label: String?, status: ConnectionStatus?) async -> ProviderConnection? { + func createProviderConnection(name: String, provider: String, auth: ProviderConnectionAuth, label: String?, status: ConnectionStatus?) async -> ProviderConnectionCreateResult { createCallCount += 1 createNameArg = name createProviderArg = provider @@ -247,28 +247,55 @@ final class ProviderConnectionClientTests: XCTestCase { func testCreateReturnsConnectionOnSuccess() async { let conn = makeConnection(name: "new-conn") - mock.createResponse = conn + mock.createResponse = .created(conn) let auth = ProviderConnectionAuth(type: "api_key", credential: "sk-test") let result = await mock.createProviderConnection(name: "new-conn", provider: "anthropic", auth: auth, label: nil, status: nil) - XCTAssertEqual(result?.name, "new-conn") + guard case .created(let created) = result else { + XCTFail("Expected .created result, got \(result)") + return + } + XCTAssertEqual(created.name, "new-conn") XCTAssertEqual(mock.createNameArg, "new-conn") XCTAssertEqual(mock.createProviderArg, "anthropic") XCTAssertEqual(mock.createAuthArg?.type, "api_key") XCTAssertEqual(mock.createAuthArg?.credential, "sk-test") } - func testCreateReturnsNilOn409NameConflict() async { - mock.createResponse = nil + func testCreateReturnsDuplicateOn409NameConflict() async { + // 409 from the daemon (a connection with this name already exists) + // surfaces as `.duplicate` so callers can show a precise message + // instead of a generic "please try again." + mock.createResponse = .duplicate let auth = ProviderConnectionAuth(type: "api_key", credential: "sk-test") let result = await mock.createProviderConnection(name: "existing", provider: "anthropic", auth: auth, label: nil, status: nil) - XCTAssertNil(result, "nil return models 409 name conflict") + guard case .duplicate = result else { + XCTFail("Expected .duplicate result, got \(result)") + return + } } - func testCreateReturnsNilOn400InvalidAuth() async { - mock.createResponse = nil + func testCreateReturnsInvalidWithMessageOn400() async { + // 400 carries the daemon's structured reason when available so the + // sheet can echo it verbatim (e.g. `Invalid provider "x". Valid: ...`). + mock.createResponse = .invalid(message: "Invalid auth configuration.") let auth = ProviderConnectionAuth(type: "api_key", credential: "") let result = await mock.createProviderConnection(name: "conn", provider: "anthropic", auth: auth, label: nil, status: nil) - XCTAssertNil(result, "nil return models 400 invalid auth") + guard case .invalid(let message) = result else { + XCTFail("Expected .invalid result, got \(result)") + return + } + XCTAssertEqual(message, "Invalid auth configuration.") + } + + func testCreateReturnsErrorOnUnknownFailure() async { + // Catch-all for network errors, 5xx, decode failures, etc. + mock.createResponse = .error + let auth = ProviderConnectionAuth(type: "api_key", credential: "sk-test") + let result = await mock.createProviderConnection(name: "conn", provider: "anthropic", auth: auth, label: nil, status: nil) + guard case .error = result else { + XCTFail("Expected .error result, got \(result)") + return + } } // MARK: - update diff --git a/clients/shared/Network/ProviderConnectionClient.swift b/clients/shared/Network/ProviderConnectionClient.swift index ec93b24d0f6..17f22a69c6b 100644 --- a/clients/shared/Network/ProviderConnectionClient.swift +++ b/clients/shared/Network/ProviderConnectionClient.swift @@ -11,11 +11,30 @@ public enum ProviderConnectionDeleteResult: Sendable { case error } +/// Result of a `POST /v1/inference/provider-connections` request. +/// +/// Differentiates the failure modes that warrant a specific user-facing +/// message — duplicate name (409) and invalid request (400) — from the +/// generic catch-all so the editor sheet can surface a precise reason +/// instead of a generic "please try again." +public enum ProviderConnectionCreateResult: Sendable { + case created(ProviderConnection) + /// 409 — a connection with the same name already exists. + case duplicate + /// 400 — invalid request. `message` carries the daemon's human-readable + /// reason (e.g. `Invalid provider "x". Valid: ...`) when present in the + /// `{ error: { message } }` envelope. May be nil if the body can't be + /// parsed; callers should provide a sensible fallback. + case invalid(message: String?) + /// Anything else (network failure, 5xx, decode error). + case error +} + public protocol ProviderConnectionClientProtocol { func listProviderConnections(provider: String?) async -> [ProviderConnection]? func getProviderConnection(name: String) async -> ProviderConnection? /// `label` and `status` are optional extras; pass `nil` to omit from the request body. - func createProviderConnection(name: String, provider: String, auth: ProviderConnectionAuth, label: String?, status: ConnectionStatus?) async -> ProviderConnection? + func createProviderConnection(name: String, provider: String, auth: ProviderConnectionAuth, label: String?, status: ConnectionStatus?) async -> ProviderConnectionCreateResult /// `status`: nil = omit from body (no change). `label`: nil outer = omit; `.some(nil)` = send null (clear); `.some("v")` = set. func updateProviderConnection(name: String, auth: ProviderConnectionAuth, status: ConnectionStatus?, label: String??) async -> ProviderConnection? func deleteProviderConnection(name: String) async -> ProviderConnectionDeleteResult @@ -82,7 +101,7 @@ public struct ProviderConnectionClient: ProviderConnectionClientProtocol { auth: ProviderConnectionAuth, label: String? = nil, status: ConnectionStatus? = nil - ) async -> ProviderConnection? { + ) async -> ProviderConnectionCreateResult { var body: [String: Any] = [ "name": name, "provider": provider, @@ -95,15 +114,39 @@ public struct ProviderConnectionClient: ProviderConnectionClientProtocol { path: "inference/provider-connections", json: body ) - guard response.isSuccess else { + switch response.statusCode { + case 200..<300: + do { + let conn = try JSONDecoder().decode(ProviderConnection.self, from: response.data) + return .created(conn) + } catch { + log.error("createProviderConnection decode: \(error.localizedDescription, privacy: .public)") + return .error + } + case 409: + return .duplicate + case 400: + return .invalid(message: Self.parseErrorMessage(from: response.data)) + default: log.warning("POST /inference/provider-connections failed: \(response.statusCode)") - return nil + return .error } - return try JSONDecoder().decode(ProviderConnection.self, from: response.data) } catch { log.error("createProviderConnection: \(error.localizedDescription, privacy: .public)") + return .error + } + } + + /// Extract `error.message` from the standard daemon error envelope + /// `{ error: { code, message, details? } }`. Returns nil when the body + /// can't be parsed or doesn't follow the envelope. + private static func parseErrorMessage(from data: Data) -> String? { + guard let json = try? JSONSerialization.jsonObject(with: data) as? [String: Any], + let error = json["error"] as? [String: Any], + let message = error["message"] as? String else { return nil } + return message } public func updateProviderConnection(