fix(all-mcp): parser-consistency cluster — int64ArgValueStrict + parseLimit + dump default cap (#22, #23, #25)#32
Open
kiki830621 wants to merge 3 commits into
Open
Conversation
#22, #23, #25) Parser-consistency cluster — closes the gap left by #8 (parseMaxMessages): - int64ArgValueStrict throws on .string("not-numeric") instead of silent nil → caller-throws-required. Closes #22 misleading-error-message. Used by both parsers (parseGetChatHistoryArgs, parseDumpChatToMarkdownArgs) for chat_id and from_message_id; the non-strict int64ArgValue stays for the 20+ direct Server.swift callsites (separate sister concern). - parseLimit modeled on parseMaxMessages: accepts .int / .string(numeric) / whole .double; throws on type-mismatch + zero/negative. Closes #25 silent fallback shape (args["limit"]?.intValue ?? N) at the parser layer. - parseDumpChatToMarkdownArgs default-5000 path now runs validateMaxMessagesCap explicitly (belt-and-suspenders). Closes #23 latent-invariant: the cap's docstring claims single source of truth but the default-5000 literal previously bypassed it. Future cap policy changes (e.g. tighten to 1000 for paid tier) will now propagate to the default path atomically.
…FromParse in catch-all (#22, #23, #25) Caller-layer side of the parser-consistency cluster: - get_chats / search_chats / search_messages / get_chat_members: args["limit"]?.intValue ?? N → try parseLimit(args, default: N). Each callsite now rejects type-mismatch + zero/negative limits at the MCP transport boundary instead of silently producing default-size results. - handleToolCall catch-all (L591): error.localizedDescription → errorResultFromParse(error). For HandlerArgError / DateParseError this surfaces the user-facing description (was generic Swift fallback string); other error types still hit the same localizedDescription path inside errorResultFromParse. Drive-by improvement so parseLimit throws are visible to MCP clients via the catch-all path (the existing dedicated parse-error do/catch blocks for get_chat_history / dump_chat_to_markdown are still in place — unchanged).
Updates testChatIdAsStringInvalidThrows to lock the #22 fix (asserts the new "chat_id must be an integer; got \"...\"" message instead of the pre-#22 misleading "chat_id is required") and adds 7 new tests: - testFromMessageIdAsStringInvalidThrows — #22 parity for from_message_id - testDumpChatIdAsStringInvalidThrows — #22 parity for parseDumpChatToMarkdownArgs - testParseDumpDefaultMaxMessagesRespectsCap — #23 structural invariant guard (default-5000 path now flows through validateMaxMessagesCap) - testLimitRejectsInvalidString — #25 non-numeric string rejected - testLimitAcceptsWholeNumberDouble — #25 JS/Python encoder parity (.double(50.0) accepted, same shape as parseMaxMessages from #8) - testLimitRejectsZeroOrNegative — #25 positive-only invariant - testLimitAcceptsNumericString — #25 legacy MCP client parity Test count: 204 → 212 (no failures).
This was referenced May 25, 2026
Contributor
Author
Verify Report — PR #32Engine5 general-purpose Agents (Claude reviewers) + Codex (gpt-5.5 xhigh) = 6-AI ensemble. All 6 produced findings (2 reviewers had socket-error first attempt, recovered via Step 2.5b retry). Aggregate🔴 FAIL — 5 P1 blockers + 3 P2 in-scope-fix + 1 P3 follow-up. PR refs: #22 #23 #25 (plus cross-refs #8 #11 #12 from commit history) Findings (cross-source merged)
Process Gaps
Devil's Advocate Verdict
Recommended Action Plan
Verify-gated checklist updatePR description's checklist says: Verify ran, found 5 P1 blockers. Checklist should remain ☐ until P1s addressed. Source files:
🤖 Generated by |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Refs #22 #23 #25
Summary
Parser-consistency cluster — closes 3 related issues filed during the
#8/#11/#12verification batch (parseMaxMessagesstrictness wave, 2026-04-26 / -27). Same root-cause shape (silent fallback / type-strictness) at the helper layer, propagated to 5 callsites + 6 new tests.#22 —
chat_idtype-mismatch message misleadsint64ArgValuereturns nil on.string("not-a-number")→ caller throws "X is required" (key was present, value was the wrong type). Fix: newint64ArgValueStrictthrows with quoted value for debug clarity. Used by both parsers; non-strictint64ArgValuestays for the 20+ direct Server.swift callsites (sister concern).#23 —
parseDumpChatToMarkdownArgsdefault-5000 bypasses capparseMaxMessagesonly validates explicit args; the literal?? 5000fallback silently bypassesvalidateMaxMessagesCapdespite the cap's docstring claiming single source of truth. Fix: belt-and-suspenders explicit cap call after the default fallback.#25 —
parseLimitpattern not appliedServer.swift× 4 +HandlerArgs.swift:81still useargs["limit"]?.intValue ?? N. Fix: newparseLimithelper modeled onparseMaxMessages; rejects non-numeric strings + zero/negative; accepts whole-number doubles per MCP SDK convention.Changes
HandlerArgs.swiftint64ArgValueStrict, +parseLimit, +validateMaxMessagesCapcall on dump defaultServer.swifttry parseLimit(args, default: N); catch-all →errorResultFromParse(error)(drive-by, so HandlerArgError surfaces user-friendly description)ServerHandlerLogicTests.swifttestChatIdAsStringInvalidThrows(locks #22 fix) + 7 new testsCommits
77bf279feat: add int64ArgValueStrict + parseLimit + dump default cap5ae4a8erefactor: swap limit callers + use errorResultFromParse in catch-all63b74dftest: cover parser-consistency clusterChecklist
swift build -c release✓)swift test --skip E2ETests→ 212 tests, 0 failures, was 204)/idd-verify --pr <N>)/idd-close #22 #23 #25after mergeSister concerns surfaced during implementation
int64ArgValuenon-strict variant still serves 20+ direct Server.swift callsites with the same chat_id type-mismatch error message misleads (silently shows 'is required' instead of type error) #22 bug shape (get_chat,send_message,edit_message, etc.). Out of scope for this cluster — different blast radius would mean +20trypropagation. Will be filed as new issue at Step 5.7 sister bug sweep.Generated by
/idd-implementon PR path. Do NOT add a GitHub close trailer (Closes/Fixes/Resolves) — IDD discipline requires manual/idd-closeafter merge to enforce checklist gate + closing summary.