diff --git a/che-telegram-all-mcp/Sources/CheTelegramAllMCPCore/HandlerArgs.swift b/che-telegram-all-mcp/Sources/CheTelegramAllMCPCore/HandlerArgs.swift index 77d0fdc..e9f0633 100644 --- a/che-telegram-all-mcp/Sources/CheTelegramAllMCPCore/HandlerArgs.swift +++ b/che-telegram-all-mcp/Sources/CheTelegramAllMCPCore/HandlerArgs.swift @@ -75,11 +75,11 @@ internal func errorResultFromParse(_ error: Error) -> CallTool.Result { /// - `since_date` / `until_date` parse via `parseISODate` / `parseUntilDate` /// which throw `DateParseError` on invalid format (#5). internal func parseGetChatHistoryArgs(_ args: [String: Value]) throws -> GetChatHistoryArgs { - guard let chatId = int64ArgValue(args, "chat_id") else { + guard let chatId = try int64ArgValueStrict(args, "chat_id") else { throw HandlerArgError(message: "chat_id is required") } - let limit = args["limit"]?.intValue ?? 50 - let fromMessageId = int64ArgValue(args, "from_message_id") ?? 0 + let limit = try parseLimit(args, default: 50) + let fromMessageId = try int64ArgValueStrict(args, "from_message_id") ?? 0 let sinceDate = try parseISODate(args["since_date"]?.stringValue) let untilDate = try parseUntilDate(args["until_date"]?.stringValue) @@ -127,13 +127,20 @@ internal struct DumpChatToMarkdownArgs { /// `parseUntilDate` which throw `DateParseError` on invalid format /// - `self_label` defaults to `"我"` (Mandarin "I/me") internal func parseDumpChatToMarkdownArgs(_ args: [String: Value]) throws -> DumpChatToMarkdownArgs { - guard let chatId = int64ArgValue(args, "chat_id") else { + guard let chatId = try int64ArgValueStrict(args, "chat_id") else { throw HandlerArgError(message: "chat_id is required") } guard let outputPath = args["output_path"]?.stringValue else { throw HandlerArgError(message: "output_path is required") } + // #23: belt-and-suspenders cap on the default-5000 fallback path. + // `parseMaxMessages` only validates explicit args; without this call, + // the literal 5000 default would silently bypass `validateMaxMessagesCap` + // (cap = single source of truth per #13). If a future cap policy + // tightens to e.g. 1000 for paid tier, this line ensures the default + // path is also subject to the new ceiling. let maxMessages = try parseMaxMessages(args) ?? 5000 + try validateMaxMessagesCap(maxMessages) let sinceDate = try parseISODate(args["since_date"]?.stringValue) let untilDate = try parseUntilDate(args["until_date"]?.stringValue) @@ -223,3 +230,63 @@ internal func int64ArgValue(_ args: [String: Value], _ key: String) -> Int64? { if let s = value.stringValue { return Int64(s) } return nil } + +/// Strict throwing variant of `int64ArgValue` used by the parsers in this +/// module. Closes #22: the non-strict `int64ArgValue` silently returns nil +/// on `.string("not-numeric")`, causing callers to throw the misleading +/// "X is required" (the key WAS present, the value was the wrong type). +/// +/// Resolution order (uses MCP SDK's `Int(_:strict:false)` for parity with +/// `parseMaxMessages`): +/// 1. Key absent → return `nil` (caller decides default vs. throw) +/// 2. Explicit `.null` → return `nil` (treat as absent) +/// 3. `.int(n)` → `Int64(n)` +/// 4. `.double(d)` where `Int(exactly: d) != nil` → `Int64(d)` (whole-number +/// doubles like `12345.0` accepted; matters for JS/Python encoders that +/// emit integers as doubles per JSON spec) +/// 5. `.string(s)` → `Int64(s)` strict base-10 +/// 6. Anything else (`.bool`, `.array`, `.object`, fractional `.double`, +/// non-numeric `.string`) → throw `HandlerArgError` with quoted value +/// for debug clarity +internal func int64ArgValueStrict(_ args: [String: Value], _ key: String) throws -> Int64? { + guard let raw = args[key] else { return nil } + if case .null = raw { return nil } + if let n = Int(raw, strict: false) { + return Int64(n) + } + if let s = raw.stringValue { + throw HandlerArgError( + message: "\(key) must be an integer; got \"\(s)\"" + ) + } + throw HandlerArgError(message: "\(key) must be an integer") +} + +/// Parse and validate the optional `limit` arg with caller-supplied default. +/// +/// Closes #25: previously +/// `args["limit"]?.intValue ?? defaultValue` silently fell back to the +/// default for any non-`.int` value — `.string("0")` / `.double(20.0)` / +/// non-numeric strings all silently produced the default. Mirrors the +/// `parseMaxMessages` shape (#8 A1) for consistency across numeric option +/// args. +/// +/// Resolution order via MCP SDK's `Int(_:strict:false)`: +/// 1. Key absent → return `defaultValue` (caller's default applied) +/// 2. Explicit `.null` → return `defaultValue` (treat as absent) +/// 3. `.int(n)` / whole `.double(d)` / numeric `.string(s)` → `n` +/// 4. Anything else → throw `HandlerArgError` +/// +/// Then validates `limit > 0` — zero or negative limits would silently +/// produce empty results upstream (debug hell). +internal func parseLimit(_ args: [String: Value], default defaultValue: Int) throws -> Int { + guard let raw = args["limit"] else { return defaultValue } + if case .null = raw { return defaultValue } + guard let value = Int(raw, strict: false) else { + throw HandlerArgError(message: "limit must be an integer") + } + if value <= 0 { + throw HandlerArgError(message: "limit must be positive; got \(value)") + } + return value +} diff --git a/che-telegram-all-mcp/Sources/CheTelegramAllMCPCore/Server.swift b/che-telegram-all-mcp/Sources/CheTelegramAllMCPCore/Server.swift index ed0bbcd..acdeb90 100644 --- a/che-telegram-all-mcp/Sources/CheTelegramAllMCPCore/Server.swift +++ b/che-telegram-all-mcp/Sources/CheTelegramAllMCPCore/Server.swift @@ -430,7 +430,7 @@ public final class CheTelegramAllMCPServer { // Chat Operations case "get_chats": - let limit = args["limit"]?.intValue ?? 50 + let limit = try parseLimit(args, default: 50) result = try await tdlib.getChats(limit: limit) case "get_chat": @@ -443,7 +443,7 @@ public final class CheTelegramAllMCPServer { guard let query = args["query"]?.stringValue else { return errorResult("query is required") } - let limit = args["limit"]?.intValue ?? 20 + let limit = try parseLimit(args, default: 20) result = try await tdlib.searchChats(query: query, limit: limit) // Message Operations @@ -500,7 +500,7 @@ public final class CheTelegramAllMCPServer { let query = args["query"]?.stringValue else { return errorResult("chat_id and query are required") } - let limit = args["limit"]?.intValue ?? 50 + let limit = try parseLimit(args, default: 50) result = try await tdlib.searchMessages(chatId: chatId, query: query, limit: limit) // Group Management @@ -508,7 +508,7 @@ public final class CheTelegramAllMCPServer { guard let chatId = int64ArgValue(args, "chat_id") else { return errorResult("chat_id is required") } - let limit = args["limit"]?.intValue ?? 200 + let limit = try parseLimit(args, default: 200) result = try await tdlib.getChatMembers(chatId: chatId, limit: limit) case "pin_message": @@ -589,7 +589,14 @@ public final class CheTelegramAllMCPServer { } catch TDLibClient.TDError.tdlibError(let code, let message) { return tdlibErrorResult(code: code, message: message) } catch { - return errorResult(error.localizedDescription) + // Use errorResultFromParse so HandlerArgError / DateParseError + // thrown by parseLimit / parseGetChatHistoryArgs / etc. surface + // their `description` (user-friendly message) rather than the + // generic Swift `Error.localizedDescription` fallback. Other + // error types still fall through to localizedDescription inside + // errorResultFromParse. Improves observability for the cluster + // #22 / #23 / #25 throw paths that were previously silent. + return errorResultFromParse(error) } } diff --git a/che-telegram-all-mcp/Tests/CheTelegramAllMCPTests/ServerHandlerLogicTests.swift b/che-telegram-all-mcp/Tests/CheTelegramAllMCPTests/ServerHandlerLogicTests.swift index 07e6fc3..1b94608 100644 --- a/che-telegram-all-mcp/Tests/CheTelegramAllMCPTests/ServerHandlerLogicTests.swift +++ b/che-telegram-all-mcp/Tests/CheTelegramAllMCPTests/ServerHandlerLogicTests.swift @@ -385,14 +385,45 @@ final class ServerHandlerLogicTests: XCTestCase { XCTAssertEqual(parsed.fromMessageId, 12345) } - /// Non-numeric string for required `chat_id` falls back to nil → throws - /// "chat_id is required" (the field is treated as missing). + /// #22: non-numeric string for `chat_id` now throws a type-mismatch + /// error instead of the misleading "chat_id is required". Pre-#22, + /// `int64ArgValue` silently returned nil on parse-fail, so the caller + /// threw "is required" — leading the user to check args structure + /// instead of value type. Post-#22, `int64ArgValueStrict` distinguishes + /// absent (nil → caller decides) vs. wrong type (throws with quoted + /// value for debug clarity). func testChatIdAsStringInvalidThrows() { XCTAssertThrowsError(try parseGetChatHistoryArgs([ "chat_id": .string("not-a-number"), ])) { error in XCTAssertEqual((error as? HandlerArgError)?.description, - "chat_id is required") + "chat_id must be an integer; got \"not-a-number\"") + } + } + + /// #22 parity for `from_message_id` — same type-mismatch handling via + /// `int64ArgValueStrict`. Without this test, a regression that re-narrows + /// strictness only to `chat_id` could go unnoticed. + func testFromMessageIdAsStringInvalidThrows() { + XCTAssertThrowsError(try parseGetChatHistoryArgs([ + "chat_id": .int(100), + "from_message_id": .string("garbage"), + ])) { error in + XCTAssertEqual((error as? HandlerArgError)?.description, + "from_message_id must be an integer; got \"garbage\"") + } + } + + /// #22 parity for `parseDumpChatToMarkdownArgs.chat_id` — confirms the + /// strict-helper upgrade applied to both parsers, not just + /// `parseGetChatHistoryArgs`. + func testDumpChatIdAsStringInvalidThrows() { + XCTAssertThrowsError(try parseDumpChatToMarkdownArgs([ + "chat_id": .string("not-a-number"), + "output_path": .string("/tmp/x.md"), + ])) { error in + XCTAssertEqual((error as? HandlerArgError)?.description, + "chat_id must be an integer; got \"not-a-number\"") } } @@ -525,4 +556,80 @@ final class ServerHandlerLogicTests: XCTestCase { XCTAssertEqual(components.minute, 59) XCTAssertEqual(components.second, 59) } + + // MARK: - #23: parseDumpChatToMarkdownArgs default-5000 cap + + /// #23: when `max_messages` is omitted, default 5000 still flows through + /// `validateMaxMessagesCap`. Pre-#23, the default literal 5000 silently + /// bypassed the cap (since `parseMaxMessages` only validated explicit + /// args). Currently `cap = 10_000 > 5000`, so the explicit default + /// passes; this test locks the structural invariant that the default + /// path is gated by the cap. If a future policy reduces the cap below + /// 5000, this test will (intentionally) start failing — that's the + /// signal to also update the default literal in lockstep with the cap. + func testParseDumpDefaultMaxMessagesRespectsCap() throws { + let parsed = try parseDumpChatToMarkdownArgs([ + "chat_id": .int(100), + "output_path": .string("/tmp/x.md"), + ]) + XCTAssertEqual(parsed.maxMessages, 5000, + "default max_messages must remain 5000 (and pass cap)") + } + + // MARK: - #25: parseLimit (numeric arg strictness) + + /// #25: `parseLimit` rejects non-numeric strings (parity with + /// `parseMaxMessages` #8 A1). Pre-#25, `args["limit"]?.intValue ?? 50` + /// silently fell back to 50 for `.string("not-a-number")`. + func testLimitRejectsInvalidString() { + XCTAssertThrowsError(try parseGetChatHistoryArgs([ + "chat_id": .int(100), + "limit": .string("not-a-number"), + ])) { error in + XCTAssertEqual((error as? HandlerArgError)?.description, + "limit must be an integer") + } + } + + /// #25 parity with `parseMaxMessages`: JS / Python JSON encoders emit + /// integers as doubles (`50.0`), MCP transport surfaces them as + /// `.double(50.0)`. `parseLimit` accepts whole-number doubles per + /// MCP SDK's `Int(_:strict:false)` (parity with #8 commit `f0203ac`). + func testLimitAcceptsWholeNumberDouble() throws { + let parsed = try parseGetChatHistoryArgs([ + "chat_id": .int(100), + "limit": .double(50.0), + ]) + XCTAssertEqual(parsed.limit, 50) + } + + /// #25: `parseLimit` rejects zero (would silently produce empty results + /// upstream). Mirrors `parseMaxMessages` `validateMaxMessagesCap` + /// positive-check pattern. + func testLimitRejectsZeroOrNegative() { + XCTAssertThrowsError(try parseGetChatHistoryArgs([ + "chat_id": .int(100), + "limit": .int(0), + ])) { error in + XCTAssertEqual((error as? HandlerArgError)?.description, + "limit must be positive; got 0") + } + XCTAssertThrowsError(try parseGetChatHistoryArgs([ + "chat_id": .int(100), + "limit": .int(-5), + ])) { error in + XCTAssertEqual((error as? HandlerArgError)?.description, + "limit must be positive; got -5") + } + } + + /// #25: `parseLimit` accepts numeric strings (parity with + /// `parseMaxMessages` legacy MCP client behavior). + func testLimitAcceptsNumericString() throws { + let parsed = try parseGetChatHistoryArgs([ + "chat_id": .int(100), + "limit": .string("75"), + ]) + XCTAssertEqual(parsed.limit, 75) + } }