From 42e9e8954df4d1f5c36f141d3c08b89505835158 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ng=C3=B4=20Qu=E1=BB=91c=20=C4=90=E1=BA=A1t?= Date: Fri, 15 May 2026 16:12:01 +0700 Subject: [PATCH 1/4] fix(ai-chat): sanitize tool schemas for Gemini function_declarations --- CHANGELOG.md | 1 + TablePro/Core/AI/GeminiProvider.swift | 54 +++++++++++- .../Core/AI/GeminiProviderEncodingTests.swift | 85 +++++++++++++++++++ 3 files changed, 139 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1ea031b39..ae22058ae 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -29,6 +29,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- AI Chat: Gemini provider now sends tool schemas with `additionalProperties` stripped and optional fields rewritten from `type: [X, null]` to `type: X, nullable: true`, fixing 400 errors when sending a message with tools enabled. - MySQL/MariaDB: `BIT(N)` columns now display as decimal numbers (`0`, `1`, `255`) instead of raw bytes that showed up as control characters like `^A` in the data grid. (#1272) - ClickHouse, BigQuery, CloudflareD1, LibSQL, Etcd, and DynamoDB: long-running queries no longer fail at 30 seconds when Settings > Query timeout is set higher. The HTTP transport now uses the configured query timeout plus a 30-second grace, so the server's `max_execution_time` (or equivalent) fires before the client gives up. Setting "No limit" raises the transport ceiling to 1 hour. (#1267) - AI Chat: DeepSeek V4 thinking content (`reasoning_content`) is now captured during streaming and passed back in subsequent turns, fixing 400 errors when using deepseek-v4-pro or deepseek-v4-flash. diff --git a/TablePro/Core/AI/GeminiProvider.swift b/TablePro/Core/AI/GeminiProvider.swift index 7a726847f..aa8b5bd0e 100644 --- a/TablePro/Core/AI/GeminiProvider.swift +++ b/TablePro/Core/AI/GeminiProvider.swift @@ -184,7 +184,8 @@ final class GeminiProvider: ChatTransport { "name": tool.name, "description": tool.description ] - entry["parameters"] = try tool.inputSchema.jsonObject() + let sanitized = Self.sanitizeSchemaForGemini(tool.inputSchema) + entry["parameters"] = try sanitized.jsonObject() return entry } body["tools"] = [["functionDeclarations": declarations]] @@ -325,6 +326,57 @@ final class GeminiProvider: ChatTransport { return "{}" } } + + // MARK: - Schema Sanitization + + /// Strip JSON Schema features Gemini's `function_declarations` API rejects. + /// Removes `additionalProperties` keys at any depth. Rewrites optional fields + /// expressed as `type: ["X", "null"]` to `type: "X"` plus `nullable: true`, + /// which is the form Gemini accepts. + static func sanitizeSchemaForGemini(_ schema: JsonValue) -> JsonValue { + switch schema { + case .object(let fields): + var rewritten: [String: JsonValue] = [:] + var nullable = false + + for (key, value) in fields { + if key == "additionalProperties" { + continue + } + if key == "type", case .array(let members) = value { + let nonNull = members.compactMap { member -> String? in + if case .string(let typeName) = member, typeName != "null" { + return typeName + } + if case .string("null") = member { + nullable = true + return nil + } + return nil + } + if nonNull.count == 1, let primary = nonNull.first { + rewritten["type"] = .string(primary) + } else { + rewritten["type"] = .array(nonNull.map(JsonValue.string)) + } + continue + } + rewritten[key] = sanitizeSchemaForGemini(value) + } + + if nullable { + rewritten["nullable"] = .bool(true) + } + + return .object(rewritten) + + case .array(let items): + return .array(items.map(sanitizeSchemaForGemini)) + + default: + return schema + } + } } /// Mutable state carried across `GeminiProvider.parseChunk` calls. diff --git a/TableProTests/Core/AI/GeminiProviderEncodingTests.swift b/TableProTests/Core/AI/GeminiProviderEncodingTests.swift index d1a4ee648..cc0d09217 100644 --- a/TableProTests/Core/AI/GeminiProviderEncodingTests.swift +++ b/TableProTests/Core/AI/GeminiProviderEncodingTests.swift @@ -93,3 +93,88 @@ struct GeminiProviderEncodingTests { #expect(contents[0]["role"] as? String == "user") } } + +@Suite("GeminiProvider schema sanitization") +struct GeminiProviderSchemaSanitizationTests { + @Test("Strips additionalProperties at any depth") + func stripsAdditionalProperties() { + let input = JsonValue.object([ + "type": .string("object"), + "additionalProperties": .bool(false), + "properties": .object([ + "nested": .object([ + "type": .string("object"), + "additionalProperties": .bool(false) + ]) + ]) + ]) + let output = GeminiProvider.sanitizeSchemaForGemini(input) + guard case .object(let root) = output else { Issue.record("expected object"); return } + #expect(root["additionalProperties"] == nil) + guard case .object(let props) = root["properties"], + case .object(let nested) = props["nested"] else { Issue.record("expected nested"); return } + #expect(nested["additionalProperties"] == nil) + } + + @Test("Converts type:[X,null] to type:X plus nullable:true") + func rewritesOptionalType() { + let input = JsonValue.object([ + "type": .array([.string("string"), .string("null")]), + "description": .string("optional field") + ]) + let output = GeminiProvider.sanitizeSchemaForGemini(input) + guard case .object(let fields) = output else { Issue.record("expected object"); return } + #expect(fields["type"] == .string("string")) + #expect(fields["nullable"] == .bool(true)) + #expect(fields["description"] == .string("optional field")) + } + + @Test("Leaves non-nullable type unchanged") + func preservesNonNullableType() { + let input = JsonValue.object([ + "type": .string("integer"), + "description": .string("count") + ]) + let output = GeminiProvider.sanitizeSchemaForGemini(input) + guard case .object(let fields) = output else { Issue.record("expected object"); return } + #expect(fields["type"] == .string("integer")) + #expect(fields["nullable"] == nil) + } + + @Test("Recurses into properties and array items") + func recursesIntoArrayItems() { + let input = JsonValue.object([ + "type": .string("array"), + "items": .object([ + "type": .array([.string("string"), .string("null")]), + "additionalProperties": .bool(false) + ]) + ]) + let output = GeminiProvider.sanitizeSchemaForGemini(input) + guard case .object(let root) = output, + case .object(let items) = root["items"] else { Issue.record("expected items object"); return } + #expect(items["type"] == .string("string")) + #expect(items["nullable"] == .bool(true)) + #expect(items["additionalProperties"] == nil) + } + + @Test("Preserves enum and required fields") + func preservesEnumAndRequired() { + let input = JsonValue.object([ + "type": .string("object"), + "required": .array([.string("id")]), + "properties": .object([ + "id": .object([ + "type": .string("string"), + "enum": .array([.string("a"), .string("b")]) + ]) + ]) + ]) + let output = GeminiProvider.sanitizeSchemaForGemini(input) + guard case .object(let root) = output else { Issue.record("expected object"); return } + #expect(root["required"] == .array([.string("id")])) + guard case .object(let props) = root["properties"], + case .object(let id) = props["id"] else { Issue.record("expected id"); return } + #expect(id["enum"] == .array([.string("a"), .string("b")])) + } +} From 3839a2a9bbbab6663ea00b896ba267d058ce32b6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ng=C3=B4=20Qu=E1=BB=91c=20=C4=90=E1=BA=A1t?= Date: Fri, 15 May 2026 16:42:03 +0700 Subject: [PATCH 2/4] fix(ai-chat): round-trip Gemini thoughtSignature on tool calls --- CHANGELOG.md | 1 + TablePro/Core/AI/Chat/ChatTransport.swift | 2 +- TablePro/Core/AI/Chat/ChatTurn.swift | 17 +++++++++-- TablePro/Core/AI/GeminiProvider.swift | 14 ++++++++-- TablePro/Resources/Localizable.xcstrings | 9 ++++++ .../AIChatViewModel+Streaming.swift | 21 +++++++++++--- .../AIChatViewModel+ToolApproval.swift | 20 +++++++++++-- .../AI/AnthropicProviderParserTests.swift | 2 +- .../Core/AI/GeminiProviderEncodingTests.swift | 28 +++++++++++++++++++ .../Core/AI/GeminiProviderParserTests.swift | 4 +-- .../OpenAICompatibleProviderParserTests.swift | 2 +- .../OpenAIResponsesProviderParserTests.swift | 2 +- 12 files changed, 104 insertions(+), 18 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ae22058ae..0299e5b7a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -30,6 +30,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed - AI Chat: Gemini provider now sends tool schemas with `additionalProperties` stripped and optional fields rewritten from `type: [X, null]` to `type: X, nullable: true`, fixing 400 errors when sending a message with tools enabled. +- AI Chat: Gemini provider now round-trips `thoughtSignature` on function calls, fixing the second-round 400 error after a tool runs. - MySQL/MariaDB: `BIT(N)` columns now display as decimal numbers (`0`, `1`, `255`) instead of raw bytes that showed up as control characters like `^A` in the data grid. (#1272) - ClickHouse, BigQuery, CloudflareD1, LibSQL, Etcd, and DynamoDB: long-running queries no longer fail at 30 seconds when Settings > Query timeout is set higher. The HTTP transport now uses the configured query timeout plus a 30-second grace, so the server's `max_execution_time` (or equivalent) fires before the client gives up. Setting "No limit" raises the transport ceiling to 1 hour. (#1267) - AI Chat: DeepSeek V4 thinking content (`reasoning_content`) is now captured during streaming and passed back in subsequent turns, fixing 400 errors when using deepseek-v4-pro or deepseek-v4-flash. diff --git a/TablePro/Core/AI/Chat/ChatTransport.swift b/TablePro/Core/AI/Chat/ChatTransport.swift index 7dd0bf3a5..e5843cabe 100644 --- a/TablePro/Core/AI/Chat/ChatTransport.swift +++ b/TablePro/Core/AI/Chat/ChatTransport.swift @@ -69,7 +69,7 @@ struct ChatToolSpec: Codable, Equatable, Sendable { enum ChatStreamEvent: Sendable { case textDelta(String) - case toolUseStart(id: String, name: String) + case toolUseStart(id: String, name: String, providerMetadata: [String: String]? = nil) case toolUseDelta(id: String, inputJSONDelta: String) case toolUseEnd(id: String) case usage(AITokenUsage) diff --git a/TablePro/Core/AI/Chat/ChatTurn.swift b/TablePro/Core/AI/Chat/ChatTurn.swift index feb56a647..075988739 100644 --- a/TablePro/Core/AI/Chat/ChatTurn.swift +++ b/TablePro/Core/AI/Chat/ChatTurn.swift @@ -396,12 +396,23 @@ struct ToolUseBlock: Codable, Equatable, Sendable { let name: String let input: JsonValue var approvalState: ToolApprovalState + /// Opaque provider-specific data that must round-trip with the call (e.g., Gemini's + /// `thoughtSignature` required when echoing a function call alongside its response). + /// Keys are provider-defined; unknown providers ignore them. + var providerMetadata: [String: String]? - init(id: String, name: String, input: JsonValue, approvalState: ToolApprovalState = .approved) { + init( + id: String, + name: String, + input: JsonValue, + approvalState: ToolApprovalState = .approved, + providerMetadata: [String: String]? = nil + ) { self.id = id self.name = name self.input = input self.approvalState = approvalState + self.providerMetadata = providerMetadata } init(from decoder: Decoder) throws { @@ -410,6 +421,7 @@ struct ToolUseBlock: Codable, Equatable, Sendable { name = try container.decode(String.self, forKey: .name) input = try container.decode(JsonValue.self, forKey: .input) approvalState = try container.decodeIfPresent(ToolApprovalState.self, forKey: .approvalState) ?? .approved + providerMetadata = try container.decodeIfPresent([String: String].self, forKey: .providerMetadata) } func encode(to encoder: Encoder) throws { @@ -418,10 +430,11 @@ struct ToolUseBlock: Codable, Equatable, Sendable { try container.encode(name, forKey: .name) try container.encode(input, forKey: .input) try container.encode(approvalState, forKey: .approvalState) + try container.encodeIfPresent(providerMetadata, forKey: .providerMetadata) } private enum CodingKeys: String, CodingKey { - case id, name, input, approvalState + case id, name, input, approvalState, providerMetadata } } diff --git a/TablePro/Core/AI/GeminiProvider.swift b/TablePro/Core/AI/GeminiProvider.swift index aa8b5bd0e..77fe732e8 100644 --- a/TablePro/Core/AI/GeminiProvider.swift +++ b/TablePro/Core/AI/GeminiProvider.swift @@ -220,12 +220,16 @@ final class GeminiProvider: ChatTransport { continue case .toolUse(let useBlock): let argsObject = (try? useBlock.input.jsonObject()) ?? [String: Any]() - parts.append([ + var partEntry: [String: Any] = [ "functionCall": [ "name": useBlock.name, "args": argsObject ] - ]) + ] + if let signature = useBlock.providerMetadata?["thoughtSignature"], !signature.isEmpty { + partEntry["thoughtSignature"] = signature + } + parts.append(partEntry) case .toolResult(let resultBlock): let toolName = resolveToolName( forToolUseId: resultBlock.toolUseId, @@ -296,7 +300,11 @@ final class GeminiProvider: ChatTransport { let id = idGenerator() let argsObject = functionCall["args"] ?? [String: Any]() let argsString = encodeArgsToJSONString(argsObject) - events.append(.toolUseStart(id: id, name: name)) + var metadata: [String: String]? + if let signature = part["thoughtSignature"] as? String, !signature.isEmpty { + metadata = ["thoughtSignature": signature] + } + events.append(.toolUseStart(id: id, name: name, providerMetadata: metadata)) events.append(.toolUseDelta(id: id, inputJSONDelta: argsString)) events.append(.toolUseEnd(id: id)) } diff --git a/TablePro/Resources/Localizable.xcstrings b/TablePro/Resources/Localizable.xcstrings index f095e4b7f..f54b531f1 100644 --- a/TablePro/Resources/Localizable.xcstrings +++ b/TablePro/Resources/Localizable.xcstrings @@ -16648,6 +16648,9 @@ }, "Drop Database…" : { + }, + "Drop Failed" : { + }, "Drop Foreign Table" : { @@ -37858,6 +37861,9 @@ } } } + }, + "Remove “%@”?" : { + }, "Remove %@?" : { "localizations" : { @@ -47038,6 +47044,9 @@ }, "The previous code wasn't accepted. Wait for your authenticator to refresh, then enter the new code." : { + }, + "The provider configuration and stored API key will be deleted." : { + }, "The selected backup file is not readable." : { diff --git a/TablePro/ViewModels/AIChatViewModel+Streaming.swift b/TablePro/ViewModels/AIChatViewModel+Streaming.swift index 9a3b3d1e7..bdcc5ba73 100644 --- a/TablePro/ViewModels/AIChatViewModel+Streaming.swift +++ b/TablePro/ViewModels/AIChatViewModel+Streaming.swift @@ -20,6 +20,7 @@ extension AIChatViewModel { let toolUseOrder: [String] let toolUseNames: [String: String] let toolUseInputs: [String: String] + let toolUseMetadata: [String: [String: String]] let cancelled: Bool } @@ -144,7 +145,8 @@ extension AIChatViewModel { let assembled = Self.assembleToolUseBlocks( order: round.toolUseOrder, names: round.toolUseNames, - inputs: round.toolUseInputs + inputs: round.toolUseInputs, + metadata: round.toolUseMetadata ) let context = await MainActor.run { ChatToolContext( @@ -244,6 +246,7 @@ extension AIChatViewModel { var toolUseOrder: [String] = [] var toolUseNames: [String: String] = [:] var toolUseInputs: [String: String] = [:] + var toolUseMetadata: [String: [String: String]] = [:] var reasoningIDMap: [String: UUID] = [:] let flushInterval: ContinuousClock.Duration = .milliseconds(150) var lastFlushTime: ContinuousClock.Instant = .now @@ -255,7 +258,7 @@ extension AIChatViewModel { pendingContent += token case .usage(let usage): pendingUsage = usage - case .toolUseStart(let id, let name): + case .toolUseStart(let id, let name, let providerMetadata): if !pendingContent.isEmpty { await self.flushPending(content: pendingContent, usage: pendingUsage, into: assistantID) pendingContent = "" @@ -270,6 +273,9 @@ extension AIChatViewModel { toolUseInputs[id] = "" } toolUseNames[id] = name + if let providerMetadata, !providerMetadata.isEmpty { + toolUseMetadata[id] = providerMetadata + } case .toolUseDelta(let id, let inputJSONDelta): toolUseInputs[id, default: ""] += inputJSONDelta case .toolUseEnd: @@ -309,6 +315,7 @@ extension AIChatViewModel { toolUseOrder: toolUseOrder, toolUseNames: toolUseNames, toolUseInputs: toolUseInputs, + toolUseMetadata: toolUseMetadata, cancelled: Task.isCancelled ) } @@ -465,7 +472,8 @@ extension AIChatViewModel { nonisolated static func assembleToolUseBlocks( order: [String], names: [String: String], - inputs: [String: String] + inputs: [String: String], + metadata: [String: [String: String]] = [:] ) -> [ToolUseBlock] { order.compactMap { id -> ToolUseBlock? in guard let name = names[id] else { return nil } @@ -479,7 +487,12 @@ extension AIChatViewModel { } else { inputValue = .object([:]) } - return ToolUseBlock(id: id, name: name, input: inputValue) + return ToolUseBlock( + id: id, + name: name, + input: inputValue, + providerMetadata: metadata[id] + ) } } diff --git a/TablePro/ViewModels/AIChatViewModel+ToolApproval.swift b/TablePro/ViewModels/AIChatViewModel+ToolApproval.swift index 07f48bfe0..33a350353 100644 --- a/TablePro/ViewModels/AIChatViewModel+ToolApproval.swift +++ b/TablePro/ViewModels/AIChatViewModel+ToolApproval.swift @@ -31,7 +31,13 @@ extension AIChatViewModel { guard let self else { return assembledBlocks } let initial = assembledBlocks.map { block -> ToolUseBlock in let state = self.computeInitialApprovalState(for: block.name) - return ToolUseBlock(id: block.id, name: block.name, input: block.input, approvalState: state) + return ToolUseBlock( + id: block.id, + name: block.name, + input: block.input, + approvalState: state, + providerMetadata: block.providerMetadata + ) } self.appendPendingToolUseBlocks(initial, assistantID: assistantID) return initial @@ -60,7 +66,11 @@ extension AIChatViewModel { self?.updateApprovalState(blockID: block.id, newState: finalState, assistantID: assistantID) } resolved.append(ToolUseBlock( - id: block.id, name: block.name, input: block.input, approvalState: finalState + id: block.id, + name: block.name, + input: block.input, + approvalState: finalState, + providerMetadata: block.providerMetadata )) } return resolved @@ -142,7 +152,11 @@ extension AIChatViewModel { ) async { let initialState = computeInitialApprovalState(for: block.name) let pendingBlock = ToolUseBlock( - id: block.id, name: block.name, input: block.input, approvalState: initialState + id: block.id, + name: block.name, + input: block.input, + approvalState: initialState, + providerMetadata: block.providerMetadata ) appendPendingToolUseBlocks([pendingBlock], assistantID: assistantID) diff --git a/TableProTests/Core/AI/AnthropicProviderParserTests.swift b/TableProTests/Core/AI/AnthropicProviderParserTests.swift index 428381777..34a3a3abf 100644 --- a/TableProTests/Core/AI/AnthropicProviderParserTests.swift +++ b/TableProTests/Core/AI/AnthropicProviderParserTests.swift @@ -41,7 +41,7 @@ struct AnthropicProviderParserTests { ] ], state: &state) #expect(events.count == 1) - if case .toolUseStart(let id, let name) = events.first { + if case .toolUseStart(let id, let name, _) = events.first { #expect(id == "toolu_abc") #expect(name == "list_tables") } else { diff --git a/TableProTests/Core/AI/GeminiProviderEncodingTests.swift b/TableProTests/Core/AI/GeminiProviderEncodingTests.swift index cc0d09217..3b75ce8b7 100644 --- a/TableProTests/Core/AI/GeminiProviderEncodingTests.swift +++ b/TableProTests/Core/AI/GeminiProviderEncodingTests.swift @@ -51,6 +51,34 @@ struct GeminiProviderEncodingTests { #expect(args?["connection_id"] as? String == "abc") } + @Test("toolUse echoes thoughtSignature alongside functionCall when present") + func toolUseRoundTripsThoughtSignature() throws { + let toolUse = ToolUseBlock( + id: "call_1", + name: "list_tables", + input: .object([:]), + providerMetadata: ["thoughtSignature": "sig-abc-123"] + ) + let turn = ChatTurnWire(role: .assistant, blocks: [.toolUse(toolUse)]) + let encoded = try #require(makeProvider().encodeTurn(turn, priorTurns: [])) + let parts = encoded["parts"] as? [[String: Any]] + #expect((parts?[0])?["thoughtSignature"] as? String == "sig-abc-123") + #expect((parts?[0])?["functionCall"] != nil) + } + + @Test("toolUse without thoughtSignature omits the field") + func toolUseOmitsSignatureWhenAbsent() throws { + let toolUse = ToolUseBlock( + id: "call_1", + name: "list_tables", + input: .object([:]) + ) + let turn = ChatTurnWire(role: .assistant, blocks: [.toolUse(toolUse)]) + let encoded = try #require(makeProvider().encodeTurn(turn, priorTurns: [])) + let parts = encoded["parts"] as? [[String: Any]] + #expect((parts?[0])?["thoughtSignature"] == nil) + } + @Test("toolResult resolves the originating tool name from prior turns") func toolResultLookupAcrossTurns() throws { let toolUse = ToolUseBlock(id: "call_1", name: "list_tables", input: .object([:])) diff --git a/TableProTests/Core/AI/GeminiProviderParserTests.swift b/TableProTests/Core/AI/GeminiProviderParserTests.swift index f377d2039..cd66a0a71 100644 --- a/TableProTests/Core/AI/GeminiProviderParserTests.swift +++ b/TableProTests/Core/AI/GeminiProviderParserTests.swift @@ -49,7 +49,7 @@ struct GeminiProviderParserTests { ]] ], state: &state) #expect(events.count == 3) - if case .toolUseStart(let id, let name) = events[0] { + if case .toolUseStart(let id, let name, _) = events[0] { #expect(id == stableID) #expect(name == "list_tables") } else { @@ -156,7 +156,7 @@ struct GeminiProviderParserTests { return "id-\(counter)" }) let starts = events.compactMap { event -> String? in - if case .toolUseStart(let id, _) = event { return id } + if case .toolUseStart(let id, _, _) = event { return id } return nil } #expect(starts == ["id-0", "id-1"]) diff --git a/TableProTests/Core/AI/OpenAICompatibleProviderParserTests.swift b/TableProTests/Core/AI/OpenAICompatibleProviderParserTests.swift index 0689fa912..391509477 100644 --- a/TableProTests/Core/AI/OpenAICompatibleProviderParserTests.swift +++ b/TableProTests/Core/AI/OpenAICompatibleProviderParserTests.swift @@ -42,7 +42,7 @@ struct OpenAICompatibleProviderParserTests { ]] ], state: &state) #expect(result.events.count == 1) - if case .toolUseStart(let id, let name) = result.events.first { + if case .toolUseStart(let id, let name, _) = result.events.first { #expect(id == "call_abc") #expect(name == "list_tables") } else { diff --git a/TableProTests/Core/AI/OpenAIResponsesProviderParserTests.swift b/TableProTests/Core/AI/OpenAIResponsesProviderParserTests.swift index a7852f8d2..2dc2fb3ed 100644 --- a/TableProTests/Core/AI/OpenAIResponsesProviderParserTests.swift +++ b/TableProTests/Core/AI/OpenAIResponsesProviderParserTests.swift @@ -93,7 +93,7 @@ struct OpenAIResponsesProviderParserTests { "name": "execute_query" ] ], state: &state) - guard case .toolUseStart(let id, let name) = events.first else { + guard case .toolUseStart(let id, let name, _) = events.first else { Issue.record("expected toolUseStart; got \(events)") return } From c3959f314254369d59f521633956390a76778698 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ng=C3=B4=20Qu=E1=BB=91c=20=C4=90=E1=BA=A1t?= Date: Fri, 15 May 2026 16:50:05 +0700 Subject: [PATCH 3/4] fix(ai-chat)!: always require user approval for destructive operations --- CHANGELOG.md | 4 ++ .../AIChatViewModel+ToolApproval.swift | 23 ++++++++++- .../AI/DestructiveToolApprovalTests.swift | 39 +++++++++++++++++++ 3 files changed, 65 insertions(+), 1 deletion(-) create mode 100644 TableProTests/Core/AI/DestructiveToolApprovalTests.swift diff --git a/CHANGELOG.md b/CHANGELOG.md index 0299e5b7a..ac1e73835 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,6 +27,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Drop database now uses the native confirmation dialog instead of a custom sheet. - Add competitive tracking docs sourced from top TablePlus issues. +### Security + +- AI Chat: destructive operations (DROP, TRUNCATE, ALTER...DROP) now always require user approval before executing. Silent safe-mode and "Always Allow" no longer bypass the confirmation for `confirm_destructive_operation`. Previously an AI could drop tables without the user seeing any prompt when the connection was in Silent mode. + ### Fixed - AI Chat: Gemini provider now sends tool schemas with `additionalProperties` stripped and optional fields rewritten from `type: [X, null]` to `type: X, nullable: true`, fixing 400 errors when sending a message with tools enabled. diff --git a/TablePro/ViewModels/AIChatViewModel+ToolApproval.swift b/TablePro/ViewModels/AIChatViewModel+ToolApproval.swift index 33a350353..e7f1bc258 100644 --- a/TablePro/ViewModels/AIChatViewModel+ToolApproval.swift +++ b/TablePro/ViewModels/AIChatViewModel+ToolApproval.swift @@ -78,9 +78,25 @@ extension AIChatViewModel { @MainActor func computeInitialApprovalState(for toolName: String) -> ToolApprovalState { - if !ChatToolRegistry.shared.requiresApproval(toolName: toolName) { + let tool = ChatToolRegistry.shared.tool(named: toolName) + let toolMode = tool?.mode + + if toolMode == .readOnly { return .approved } + + // Destructive operations (`.agentOnly`) always require user approval. + // Safe-mode level and "Always Allow" cannot bypass them — the AI must not + // be able to drop tables, truncate, or alter-drop without an explicit click. + if toolMode == .agentOnly { + if let connection, connection.safeModeLevel.blocksAllWrites { + return .denied(reason: String( + localized: "Connection is read-only. Destructive operations are not permitted." + )) + } + return .pending + } + if let connection, connection.aiAlwaysAllowedTools.contains(toolName) { return .approved } @@ -119,6 +135,11 @@ extension AIChatViewModel { @MainActor func persistAlwaysAllowed(toolName: String) { + // Refuse to persist Always Allow for destructive operations. + // Each DROP/TRUNCATE/ALTER...DROP must be confirmed individually. + if ChatToolRegistry.shared.tool(named: toolName)?.mode == .agentOnly { + return + } guard var current = connection else { return } guard !current.aiAlwaysAllowedTools.contains(toolName) else { return } current.aiAlwaysAllowedTools.insert(toolName) diff --git a/TableProTests/Core/AI/DestructiveToolApprovalTests.swift b/TableProTests/Core/AI/DestructiveToolApprovalTests.swift new file mode 100644 index 000000000..072a9ea22 --- /dev/null +++ b/TableProTests/Core/AI/DestructiveToolApprovalTests.swift @@ -0,0 +1,39 @@ +// +// DestructiveToolApprovalTests.swift +// TableProTests +// + +import Foundation +@testable import TablePro +import Testing + +@Suite("Destructive tool approval contract") +struct DestructiveToolApprovalTests { + @Test("ConfirmDestructiveOperationChatTool is agentOnly mode") + func toolIsAgentOnly() { + let tool = ConfirmDestructiveOperationChatTool() + #expect(tool.mode == .agentOnly) + } + + @Test("agentOnly mode requires approval") + func agentOnlyRequiresApproval() { + #expect(ChatToolMode.agentOnly.requiresApproval == true) + } + + @Test("readOnly mode does not require approval") + func readOnlyDoesNotRequireApproval() { + #expect(ChatToolMode.readOnly.requiresApproval == false) + } + + @Test("write mode requires approval") + func writeRequiresApproval() { + #expect(ChatToolMode.write.requiresApproval == true) + } + + @Test("agentOnly tools are only allowed in agent chat mode") + func agentOnlyOnlyInAgentMode() { + #expect(ChatToolMode.agentOnly.isAllowed(in: .agent) == true) + #expect(ChatToolMode.agentOnly.isAllowed(in: .ask) == false) + #expect(ChatToolMode.agentOnly.isAllowed(in: .edit) == false) + } +} From 0451c268525d2f1822b2a44796dc25070fdde0a1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ng=C3=B4=20Qu=E1=BB=91c=20=C4=90=E1=BA=A1t?= Date: Fri, 15 May 2026 16:59:29 +0700 Subject: [PATCH 4/4] fix(ai-chat): sanitize tool schemas for GitHub Copilot LSP --- CHANGELOG.md | 1 + .../Core/AI/Chat/ChatToolSpec+Copilot.swift | 78 ++++++++- .../AI/CopilotSchemaSanitizationTests.swift | 165 ++++++++++++++++++ 3 files changed, 238 insertions(+), 6 deletions(-) create mode 100644 TableProTests/Core/AI/CopilotSchemaSanitizationTests.swift diff --git a/CHANGELOG.md b/CHANGELOG.md index ac1e73835..c4b9aaa95 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -35,6 +35,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - AI Chat: Gemini provider now sends tool schemas with `additionalProperties` stripped and optional fields rewritten from `type: [X, null]` to `type: X, nullable: true`, fixing 400 errors when sending a message with tools enabled. - AI Chat: Gemini provider now round-trips `thoughtSignature` on function calls, fixing the second-round 400 error after a tool runs. +- AI Chat: GitHub Copilot tool registration was failing with "Expected string" schema validation errors. Optional fields now register with `type: "string"` instead of `type: ["string", "null"]` and are excluded from `required`, which Copilot's LSP validator accepts. - MySQL/MariaDB: `BIT(N)` columns now display as decimal numbers (`0`, `1`, `255`) instead of raw bytes that showed up as control characters like `^A` in the data grid. (#1272) - ClickHouse, BigQuery, CloudflareD1, LibSQL, Etcd, and DynamoDB: long-running queries no longer fail at 30 seconds when Settings > Query timeout is set higher. The HTTP transport now uses the configured query timeout plus a 30-second grace, so the server's `max_execution_time` (or equivalent) fires before the client gives up. Setting "No limit" raises the transport ceiling to 1 hour. (#1267) - AI Chat: DeepSeek V4 thinking content (`reasoning_content`) is now captured during streaming and passed back in subsequent turns, fixing 400 errors when using deepseek-v4-pro or deepseek-v4-flash. diff --git a/TablePro/Core/AI/Chat/ChatToolSpec+Copilot.swift b/TablePro/Core/AI/Chat/ChatToolSpec+Copilot.swift index cfdb991f6..53a8de56c 100644 --- a/TablePro/Core/AI/Chat/ChatToolSpec+Copilot.swift +++ b/TablePro/Core/AI/Chat/ChatToolSpec+Copilot.swift @@ -10,15 +10,81 @@ extension ChatToolSpec { CopilotLanguageModelToolInformation( name: name, description: description, - inputSchema: Self.normalizeForCopilot(inputSchema) + inputSchema: Self.sanitizeForCopilot(inputSchema) ) } - private static func normalizeForCopilot(_ schema: JsonValue) -> JsonValue { - guard case .object(var dict) = schema else { return schema } - if dict["required"] == nil { - dict["required"] = .array([]) + /// Copilot's LSP schema validator rejects `type: [X, "null"]` arrays. Every + /// `type` field must be a single string. Convert nullable scalars to plain + /// scalars and drop the property from the parent's `required` array so the + /// model knows it can omit the field. Recurses into nested objects and array + /// items. + static func sanitizeForCopilot(_ schema: JsonValue) -> JsonValue { + switch schema { + case .object(let fields): + return sanitizeObject(fields) + case .array(let items): + return .array(items.map(sanitizeForCopilot)) + default: + return schema } - return .object(dict) + } + + private static func sanitizeObject(_ fields: [String: JsonValue]) -> JsonValue { + var nullableKeys: Set = [] + var rewritten: [String: JsonValue] = [:] + + for (key, value) in fields { + if key == "properties", case .object(let props) = value { + var cleanedProps: [String: JsonValue] = [:] + for (propName, propValue) in props { + let (cleaned, wasNullable) = stripNullableType(propValue) + if wasNullable { + nullableKeys.insert(propName) + } + cleanedProps[propName] = sanitizeForCopilot(cleaned) + } + rewritten[key] = .object(cleanedProps) + } else { + rewritten[key] = sanitizeForCopilot(value) + } + } + + if !nullableKeys.isEmpty, case .array(let required) = rewritten["required"] { + let filtered = required.filter { entry in + if case .string(let name) = entry { return !nullableKeys.contains(name) } + return true + } + rewritten["required"] = .array(filtered) + } + + return .object(rewritten) + } + + /// Rewrites `type: [X, "null"]` to `type: X` on a property schema. + /// Also strips `null` from `enum` arrays if present. + /// Returns the cleaned schema and whether the original was nullable. + private static func stripNullableType(_ schema: JsonValue) -> (JsonValue, Bool) { + guard case .object(var fields) = schema, + case .array(let typeMembers) = fields["type"] + else { + return (schema, false) + } + + let nullCount = typeMembers.filter { $0 == .string("null") }.count + guard nullCount > 0 else { return (schema, false) } + + let nonNull = typeMembers.filter { $0 != .string("null") } + if nonNull.count == 1, let primary = nonNull.first { + fields["type"] = primary + } else { + fields["type"] = .array(nonNull) + } + + if case .array(let enumMembers) = fields["enum"] { + fields["enum"] = .array(enumMembers.filter { $0 != .null }) + } + + return (.object(fields), true) } } diff --git a/TableProTests/Core/AI/CopilotSchemaSanitizationTests.swift b/TableProTests/Core/AI/CopilotSchemaSanitizationTests.swift new file mode 100644 index 000000000..963f4f75e --- /dev/null +++ b/TableProTests/Core/AI/CopilotSchemaSanitizationTests.swift @@ -0,0 +1,165 @@ +// +// CopilotSchemaSanitizationTests.swift +// TableProTests +// + +import Foundation +@testable import TablePro +import Testing + +@Suite("Copilot schema sanitization") +struct CopilotSchemaSanitizationTests { + @Test("Converts type:[X,null] to type:X and drops the field from required") + func rewritesOptionalScalar() { + let input = JsonValue.object([ + "type": .string("object"), + "properties": .object([ + "schema": .object([ + "type": .array([.string("string"), .string("null")]), + "description": .string("optional") + ]) + ]), + "required": .array([.string("schema")]) + ]) + let output = ChatToolSpec.sanitizeForCopilot(input) + guard case .object(let root) = output, + case .object(let props) = root["properties"], + case .object(let schemaField) = props["schema"] else { + Issue.record("expected nested object") + return + } + #expect(schemaField["type"] == .string("string")) + #expect(root["required"] == .array([])) + } + + @Test("Preserves non-nullable scalars in required") + func preservesRequiredFields() { + let input = JsonValue.object([ + "type": .string("object"), + "properties": .object([ + "connection_id": .object([ + "type": .string("string"), + "description": .string("UUID") + ]) + ]), + "required": .array([.string("connection_id")]) + ]) + let output = ChatToolSpec.sanitizeForCopilot(input) + guard case .object(let root) = output else { Issue.record("expected object"); return } + #expect(root["required"] == .array([.string("connection_id")])) + } + + @Test("Strips null from enum when type was nullable") + func stripsNullFromEnum() { + let input = JsonValue.object([ + "type": .string("object"), + "properties": .object([ + "tier": .object([ + "type": .array([.string("string"), .string("null")]), + "enum": .array([.string("a"), .string("b"), .null]) + ]) + ]), + "required": .array([.string("tier")]) + ]) + let output = ChatToolSpec.sanitizeForCopilot(input) + guard case .object(let root) = output, + case .object(let props) = root["properties"], + case .object(let tier) = props["tier"] else { + Issue.record("expected tier object") + return + } + #expect(tier["type"] == .string("string")) + #expect(tier["enum"] == .array([.string("a"), .string("b")])) + } + + @Test("Mixed required and optional drop only the nullable") + func mixedRequiredAndOptional() { + let input = JsonValue.object([ + "type": .string("object"), + "properties": .object([ + "connection_id": .object([ + "type": .string("string") + ]), + "schema": .object([ + "type": .array([.string("string"), .string("null")]) + ]) + ]), + "required": .array([.string("connection_id"), .string("schema")]) + ]) + let output = ChatToolSpec.sanitizeForCopilot(input) + guard case .object(let root) = output, + case .array(let required) = root["required"] else { + Issue.record("expected required array") + return + } + #expect(required == [.string("connection_id")]) + } + + @Test("Recurses into nested object properties") + func recursesIntoNested() { + let input = JsonValue.object([ + "type": .string("object"), + "properties": .object([ + "filter": .object([ + "type": .string("object"), + "properties": .object([ + "name": .object([ + "type": .array([.string("string"), .string("null")]) + ]) + ]), + "required": .array([.string("name")]) + ]) + ]), + "required": .array([.string("filter")]) + ]) + let output = ChatToolSpec.sanitizeForCopilot(input) + guard case .object(let root) = output, + case .object(let props) = root["properties"], + case .object(let filter) = props["filter"], + case .object(let nestedProps) = filter["properties"], + case .object(let nameField) = nestedProps["name"] else { + Issue.record("expected nested name field") + return + } + #expect(nameField["type"] == .string("string")) + #expect(filter["required"] == .array([])) + } + + @Test("Real ChatToolSchemaBuilder output passes through Copilot validator shape") + func realBuilderOutputIsValid() throws { + // Simulates what ListTablesChatTool produces. + let realSchema = ChatToolSchemaBuilder.object( + properties: [ + "connection_id": ChatToolSchemaBuilder.connectionId, + "database": ChatToolSchemaBuilder.string( + description: "Database name. Omit to use current.", + optional: true + ), + "schema": ChatToolSchemaBuilder.schemaName + ] + ) + let sanitized = ChatToolSpec.sanitizeForCopilot(realSchema) + guard case .object(let root) = sanitized, + case .object(let props) = root["properties"] else { + Issue.record("expected sanitized object") + return + } + // Every type field at the property level should be a single string. + for (_, value) in props { + guard case .object(let field) = value else { continue } + if case .array = field["type"] { + Issue.record("type should not be an array after sanitization") + } + } + // Optional fields must be removed from required. + if case .array(let required) = root["required"] { + let names = required.compactMap { val -> String? in + if case .string(let name) = val { return name } + return nil + } + #expect(!names.contains("database")) + #expect(!names.contains("schema")) + #expect(names.contains("connection_id")) + } + } +}