fix(all-mcp): parser-consistency cluster — int64ArgValueStrict + parseLimit + dump default cap (#22, #23, #25)

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
+}

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,15 +500,15 @@ 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
             case "get_chat_members":
                 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)
         }
     }
 

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)
+    }
 }