fix(mcp): honor the configured row limits in export_data (#2012) - #2013
Merged
Conversation
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
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
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.
Fixes #2012.
Root cause
ExportDataToolnever readAppSettingsManager.shared.mcpat all. It was the only MCP tool that runs user SQL and returns rows while ignoring Server Configuration entirely: row limits (hardcoded 50,000 / clamp1...100_000) and query timeout (hardcoded 60s).ExecuteQueryToolis the reference-correct pattern.Git history shows both tools were born hardcoded in #825.
execute_querywas later rewired to the settings;export_datawas missed. No CHANGELOG entry, commit message, or doc ever justified a higher export ceiling.The docs already promised the fixed behavior.
docs/customization/settings.mdxdescribes Maximum row limit as "Hard cap on rows per tool call" and Default row limit as "Rows returned when a tool call sets no limit".export_datais a tool call, so the code was wrong, not the docs.The structural cause is that
MCPToolServicescarried onlyconnectionBridgeandauthPolicy, so each tool independently reached around it into the global singleton with copy-pasted boilerplate. Nothing enforced that read, leaving the same omission latent for any future tool.What changed
export_datanow follows the same policy asexecute_query: an omittedmax_rowsuses the default row limit, a supplied value is clamped to the maximum, and the configured query timeout applies.MCPLimitResolver.swift(new)max_rowsandtimeout_secondsfromMCPSettings. One policy, shared by every tool.ExportDataTool.swiftis_truncated; routed throughToolQueryExecutor.MCPSettings.swiftvalidated*accessors; clamps on decode.AppSettingsManager.swiftmcpdidSet, mirroringdataGridandhistory.SettingsValidation.swiftmcpRowLimitRange,mcpQueryTimeoutRange.MCPToolServices.swiftsettingsProviderseam. The public 2-argument init is unchanged, so no call site breaks.ExecuteQueryTool,ConfirmDestructiveOperationTool, 2 chat toolsMCPArgumentDecoder,ChatToolArgumentDecoderTwo further bugs found and fixed
A reachable crash. The settings fields are bare
TextFields with no validation, andAppSettingsManagerclamped only the auth coupling. Typing0into "Maximum row limit" madeExecuteQueryToolformclamp: 1...0, which traps. Writing the export fix in that same shape would have added a second crash site, so the resolver is total by construction and the settings clamp at both boundaries.export_datawas broken outright on SQL Server, Oracle, and Teradata. It emittedSELECT * FROM x LIMIT nregardless of dialect, which is a syntax error on those engines. The row limit is now written in each database's own syntax (TOP,FETCH FIRST,LIMIT). The issue was filed against PostgreSQL, so this surfaced only as a limit bug.Behavior change
Export's effective default drops from 50,000 to 500. Anyone relying on the old default must pass
max_rowsor raise the setting. The CHANGELOG says so plainly, andexport_datanow returnsis_truncatedso a clipped export is visible rather than silent.Tests
50 tests pass, zero failures, across 6 suites. 27 are new:
MCPLimitResolverTests(new): defaulting, clamping, a default above the maximum, and the zero/negative cases that used to trap.MCPSettingsTests: decode clamping, and that the requestable range can never invert.ExportDataToolTests: the SQL emitted for each dialect, the truncation decision, and a probe proving the tool consults the settings seam.ChatToolArgumentDecoderTests: out-of-range and non-finite numbers.ExecuteQueryToolTestsandToolsListHandlerTestswere run as regression checks. SwiftLint--strictreports 0 violations.Review
An adversarial review raised 8 findings; 7 were refuted against source. The survivor was a pre-existing
Int(double)trap on model-controlled AI-chat input, sitting on a line this diff touched and reproduced by the verifier at exit 133. Fixed here, since it is the same class of defect this change exists to eliminate.Not fixed (pre-existing, unchanged)
A multi-table export applies the limit per table, so one call can return N times the maximum. Changing that to a shared budget across tables is a separate design decision.
Follow-up
Localizable.xcstringsis deliberately not in this commit. Xcode extracts the reworded tool descriptions on its next build, and that file carries unrelated churn.