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

che-telegram-all-mcp/CHANGELOG.md

@@ -1,5 +1,35 @@
 # Changelog
 
+## [Unreleased]
+
+Parser-consistency cluster — closes the gap left by #8 (`parseMaxMessages`).
+
+### Added
+
+- **`int64ArgValueStrict` helper (#22)**: throwing variant of `int64ArgValue` for parsers in `HandlerArgs.swift`. On `.string("not-numeric")`, throws `"\(key) must be an integer; got \"\(s)\""` with quoted user input for debug clarity. On `.bool` / `.array` / `.object` / fractional `.double`, throws `"\(key) must be an integer"` (no-quote — no meaningful string form). Used by `parseGetChatHistoryArgs` for `chat_id` + `from_message_id` and by `parseDumpChatToMarkdownArgs` for `chat_id`. Non-strict `int64ArgValue` retained for ~20 direct `Server.swift` callsites tracked separately as #33.
+- **`parseLimit` helper (#25)**: modeled on `parseMaxMessages`. Rejects non-numeric strings, zero/negative, and over-`validateLimitCap` (10_000) limits. Accepts whole-number doubles per MCP SDK's `Int(_:strict:false)` (JS / Python JSON encoders emit integers as doubles per JSON spec). The default / `.null` paths are also cap-validated (parity with `parseMaxMessagesWithDefault`, #23; mutation-guarded by `testLimitNullUsesDefault` + `testLimitDefaultOverCapRejected` — closes a verify follow-up flagged by Codex + logic + devil's-advocate).
+- **`validateLimitCap` shared policy (#25 verify F4)**: 10_000 upper bound, parity with `validateMaxMessagesCap`. Throws `"limit exceeds 10_000 cap; got \(value). Use pagination instead of a single large request."` Applies via `parseLimit`.
+- **`parseMaxMessagesWithDefault(args, default:)` helper (#23 verify F2)**: variant that flows the default-value through `validateMaxMessagesCap`. Mutation-resistant: deleting the cap call on the default branch makes `testParseMaxMessagesWithDefaultAppliesCapToDefaultPath` (cap=11000 over 10_000 ceiling) fail. Replaces the earlier inline `?? 5000 + try validateMaxMessagesCap(maxMessages)` belt-and-suspenders pattern whose test was a placebo.
+
+### Fixed
+
+- **(#22) `chat_id` / `from_message_id` type-mismatch error message** — was misleading "X is required" (silent nil from non-strict `int64ArgValue`); now throws "X must be an integer; got \"...\"" with the user's raw value quoted. Parser-layer slice (3 callsites in `HandlerArgs.swift`). The remaining 20+ direct `int64ArgValue` callsites in `Server.swift` (e.g. `get_chat`, `send_message`, `pin_message`) retain the legacy silent-fallback behavior — tracked as #33 sister.
+- **(#23) `parseDumpChatToMarkdownArgs` default-5000 path bypassed `validateMaxMessagesCap`** — the cap's docstring claimed single source of truth but the literal `?? 5000` fallback silently bypassed it. Now flows through `parseMaxMessagesWithDefault`; future cap policy tightening (e.g. 1000 for paid tier) will propagate to the default path atomically.
+- **(#25) `Server.swift` 5 `limit ?? N` silent fallbacks** — `parseGetChatHistoryArgs:81`, `Server.swift:433/446/503/511` migrated to `try parseLimit(args, default: N)`. Same shape `parseMaxMessages` fixed at #8 (`.string("0")` / `.double(20.0)` no longer silent-fallback to default).
+- **(catch-all observability)** `handleToolCall` outer catch (L591) uses `errorResultFromParse(error)` instead of `errorResult(error.localizedDescription)`. `HandlerArgError` + `DateParseError` now surface their human-readable `.description` to MCP clients via the catch-all path; other error types unchanged.
+
+### Behavior changes
+
+Observable on the wire for existing MCP clients (surfaced by PR #32 verify — devil's-advocate + regression + Codex):
+
+- **`from_message_id` non-numeric now throws.** Previously a non-numeric `.string` for `from_message_id` (in `get_chat_history`) silently fell back to nil → treated as "start from latest". It now throws `from_message_id must be an integer; got "..."`. Lenient callers that relied on junk-→-latest will get an error result instead.
+- **`limit` now has a 10_000 ceiling.** No client schema advertised an upper bound before; `limit > 10_000` now throws `limit exceeds 10_000 cap`. Callers that sent very large limits must paginate.
+- **Arg-parse error message text changed.** Via `errorResultFromParse`, `HandlerArgError` / `DateParseError` now surface their `.description` (e.g. `"limit must be an integer"`) instead of Swift's generic `localizedDescription`. Clients pattern-matching on the old error strings should update.
+
+### Notes
+
+Sister bug #33 documents the residual scope (20+ Server.swift direct `int64ArgValue` callsites with the same #22 bug shape). Cluster scope intentionally narrowed to parser-layer to keep this PR's review surface manageable.
+
 ## [0.5.5] - 2026-05-09
 
 CLI fast-exit dispatcher + startup observability for `/mcp` reconnect diagnostics.

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")
     }
-    let maxMessages = try parseMaxMessages(args) ?? 5000
+    // #23: closure of the default-5000 cap-bypass invariant.
+    // `parseMaxMessages(args)` (no-default form) only validates explicit args.
+    // `parseMaxMessagesWithDefault(args, default:)` flows the default through
+    // `validateMaxMessagesCap` too — mutation-resistant: removing the cap
+    // call inside the default branch makes `testParseMaxMessagesWithDefaultAppliesCapToDefaultPath`
+    // (cap=11000 over the 10_000 ceiling) fail. If a future cap policy
+    // tightens to e.g. 1000 for paid tier, this default-5000 path will throw.
+    let maxMessages = try parseMaxMessagesWithDefault(args, default: 5000)
 
     let sinceDate = try parseISODate(args["since_date"]?.stringValue)
     let untilDate = try parseUntilDate(args["until_date"]?.stringValue)
@@ -181,6 +188,32 @@ internal func parseMaxMessages(_ args: [String: Value]) throws -> Int? {
     return value
 }
 
+/// Like `parseMaxMessages` but applies `validateMaxMessagesCap` to the
+/// fallback default too. Closes #23 properly + makes the cap invariant
+/// mutation-resistant: deleting the `try validateMaxMessagesCap(defaultValue)`
+/// call below would make `testParseMaxMessagesWithDefaultAppliesCapToDefaultPath`
+/// (pass default=11000 over 10_000 cap, expect throw) start failing.
+///
+/// Pre-#23 fix: `parseMaxMessages(args) ?? 5000` silently bypassed the cap
+/// on the default path. The previous "belt-and-suspenders" inline fix
+/// (`try validateMaxMessagesCap(maxMessages)` after `??`) was structurally
+/// correct but its test was a placebo — only asserted `maxMessages == 5000`
+/// which holds whether the cap call is present or not (5000 < 10_000).
+/// This signature extracts the default-path semantics into a function
+/// whose contract IS the cap call, making it testable independently.
+internal func parseMaxMessagesWithDefault(_ args: [String: Value], default defaultValue: Int) throws -> Int {
+    if let raw = args["max_messages"] {
+        guard let value = Int(raw, strict: false) else {
+            throw HandlerArgError(message: "max_messages must be an integer")
+        }
+        try validateMaxMessagesCap(value)
+        return value
+    }
+    // Default path also runs the cap — closes #23 latent-invariant.
+    try validateMaxMessagesCap(defaultValue)
+    return defaultValue
+}
+
 /// Sanity check the `since_date <= until_date` invariant (#10 C2).
 /// Both bounds nil-tolerant: only checks when both are provided.
 ///
@@ -208,6 +241,19 @@ internal func validateMaxMessagesCap(_ value: Int) throws {
     }
 }
 
+/// Shared `limit` cap policy. Closes the #25 verify F4 inconsistency:
+/// `parseMaxMessages` enforced a 10_000 ceiling but `parseLimit` only
+/// checked `> 0`, letting MCP callers pass `limit: 999_999_999` straight
+/// to TDLib (`getChats` then iterates one `getChat` per id). Same 10_000
+/// ceiling for parity — limits beyond that should use pagination.
+internal func validateLimitCap(_ value: Int) throws {
+    if value > 10_000 {
+        throw HandlerArgError(
+            message: "limit exceeds 10_000 cap; got \(value). Use pagination instead of a single large request."
+        )
+    }
+}
+
 /// Module-level Int64 arg extraction. Single source of truth — formerly
 /// duplicated as `int64Arg` in `Server.swift`; consolidated here per #15-C1
 /// (DRY) so any future change (e.g. trimming whitespace, rejecting hex)
@@ -223,3 +269,77 @@ 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)` numeric → `Int64(s)` strict base-10
+/// 6. `.string(s)` non-numeric → throws `"\(key) must be an integer; got \"\(s)\""`
+///    (quoted value included for debug clarity — user's raw input is shown)
+/// 7. `.bool`, `.array`, `.object`, fractional `.double` → throws `"\(key) must be an integer"`
+///    (no quoted value — these have no meaningful string form to include)
+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` (validated against the cap)
+/// 2. Explicit `.null` → return `defaultValue` (treat as absent; cap-validated)
+/// 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).
+///
+/// The `defaultValue` itself also flows through `validateLimitCap` (parity
+/// with `parseMaxMessagesWithDefault`, #23): a future callsite passing a
+/// default over the cap must throw, not silently bypass it. Mutation-resistant
+/// via `testLimitDefaultOverCapRejected`.
+internal func parseLimit(_ args: [String: Value], default defaultValue: Int) throws -> Int {
+    // Absent / explicit .null → default, but the default is still cap-validated
+    // (see docstring + #23 design parity). The .null branch is mutation-guarded
+    // by `testLimitNullUsesDefault`.
+    func cappedDefault() throws -> Int {
+        try validateLimitCap(defaultValue)
+        return defaultValue
+    }
+    guard let raw = args["limit"] else { return try cappedDefault() }
+    if case .null = raw { return try cappedDefault() }
+    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)")
+    }
+    try validateLimitCap(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)
         }
     }