fix(mcp): run SQL Server scripts from MCP, AppleScript and the assistant as GO batches and return every result set - #3123
Merged
Conversation
…ant as GO batches and return every result set
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Automations to automatically generate PRs for you. |
Signed-off-by: Ngô Quốc Đạt <datlechin@gmail.com>
Member
Author
|
Follow-up in #3124. Review found that the execution gate still refused any SQL Server script of more than one statement on all three entry points, because no caller claimed .mayRunMultiStatement. It also found that a leading GO line still reached the server through the tools that send one statement. #3124 fixes both. |
This branch was successfully deployed
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.
What was wrong
#3105 taught the query editor to send a SQL Server script as
GObatches throughexecuteBatchand show every result set. The other three ways SQL reaches a driver never learned it. MCPexecute_query, the AI assistant'sexecute_queryand AppleScriptrun queryall run throughDatabaseAccessBridge.runStatement, which knows only the one-result call (executeUserQuery/execute). On SQL Server that gave three wrong answers:DECLARE @sn ... SELECT ... SELECT ...) or a procedure call (EXEC sp_help 'dbo.orders') passed the gate as one statement and came back with only its first result set, plus the driver's note "End each statement with a semicolon to see every result".;andGOand answers "Send one statement at a time." So the SQL Server batch variables are lost between statements #3078 reporter's script, with its semicolons, could not be sent at all, and a variable cannot be declared in one call and read in the next, because it lives only in its batch.GOline before a lone statement reached the server, which answersMsg 2812: Could not find stored procedure 'GO'., andGO 3after one ran it once. Measured:SQLStatementScanner.executableText, which the old path sent, returns"GO\nSELECT 1 AS one"unchanged and"SELECT 1"for"SELECT 1\nGO 3", and Azure SQL Edge answers the first with Msg 2812.What changed
DatabaseAccessBridge.runScripttakes the route the editor takes for the same text on the same connection:QueryBatchPlannercuts it atGO, andQueryExecutionRoute.resolvepicks the path. A lone plain query keeps the bounded single path. Anything else, on a driver that sends batches whole, runs batch by batch on one lease under the same timeout race and cancellation as a statement.GO nrepeats a batch, and every result set is held tomax_rows. A batch that raises an error fails the call with the editor's own message (Batch 2/3 failed: Line 7: ...plus what stays applied) and stops the script. The session is asked before and after, as the editor asks, so a transaction the script left open is reported. A driver withoutresultSetBatches(an older SQL Server plugin) refuses a multi-statement script orGO ninstead of running part of it.ExternalStatementGate.acceptsScripts(on:): a caller that takes scripts may send several statements in one call only where the engine cuts scripts into batches atGOlines. That is SQL Server's grammar today; every other engine keeps one statement per call. The gate still classifies the whole script by its worst statement, so one write needstools:write/ External Clients Read & Write / Safe Mode, and oneDROPis refused. The gate runs before connecting, so it decides from the dialect; the driver capability is checked at run time.execute_queryand the assistant'sexecute_queryrun through it (ToolQueryExecutorwithunit: .script). The response is additive: the existing fields still describe one result, the first, withrows_affectedfor the whole script and any notice instatus_message.result_sets[]lists every result set (columns,rows,row_count,is_truncated) only when there is more than one, so a single SELECT's payload is byte-for-byte what it was. The output schema declares it. History and the audit log count rows across every result set.run queryruns through it too. Thequery resultrecord keeps its meaning and gainsresults, a list of the newresult setrecord, empty unless a script returned more than one.QueryBatchResult.followed(by:)(repetitions, the 100 result set ceiling, error positions),DatabaseDriver.answerBatch(the nil fallback),BatchRunNotice,QueryBatchPlanner.batches(in:model:grammar:)andQueryExecutionRoute.resolve(_:databaseType:sendsBatchesWhole:). The editor's behaviour is unchanged.browse_table,export_data, maintenance, server tools,confirm_destructive_operation,explain_query) keep the single-statement path.The AppleScript names were measured, not chosen. A throwaway scriptable app with the same sdef shapes, running its AppleScript against itself (osascript would stop at the Automation consent prompt): a property named
result setsbeside a record typeresult setcompiles, then AppleScript reads it as "every result set of r" and answers{}.resultsworks for item access,repeat withandset x to results of r. Inside atellblockcount of results of ranswers 0 becausecountgoes to the app, so the docs assign first.ScriptingDictionaryTestsnow refuses a property named the plural of a record type.Verification
verify.sh generate: PASS.verify.sh build: PASS, before and after rebasing onto main.verify.sh build MSSQLDriver: PASS.verify.sh linton all 24 changed Swift files: 0 violations.verify.sh docs: PASS.check-writing-style.shandcheck-docs-against-source.py: clean.scripts/ci/check-ios-shared-isolation.py: clean (no shared file touched).xcodebuild teston this machine since 21:33 ends INCONCLUSIVE with "The test runner hung before establishing connection" and 0 cases executed, including a run with no other xcodebuild on the machine (testmanagerd wedged). The test target with these suites builds with 0 errors, before and after the rebase onto main: each run got as far as launching the test host. PR CI is the first run of these suites:DatabaseAccessBridgeScriptTests(bridge with a fake batch-capable driver injected intoDatabaseManager): the SQL Server batch variables are lost between statements #3078 script goes out as one batch with the row cap and all four result sets come back;EXEC sp_helpreturns every result set;GOandGO 3cut and repeat batches and never reach the server;GObefore a lone SELECT is not sent and the SELECT keeps the bounded path; totals and an open-transaction notice land on the first result; a batch error fails the call asBatch 2/3 failed: Line 3: ...and the third batch is never sent; result sets past the ceiling are counted; a driver without batches refuses a script andGO 2.MCPScriptResultTests:result_setsand the top-level fields, the single-result payload equal to the old encoder's, the open transaction notice, the output schema, andToolQueryExecutorwithunit: .scriptend to end over an injected session.QueryBatchResultTests,ExternalStatementGateTests(which engines take scripts; a SQL Server script is still refused on a read-only connection and with aDROPin it),ScriptResultEncoderTests(results),ScriptingDictionaryTests(theresult setkeys; no property named a record type's plural).pr_mcp_sql_server_batches):MSSQL_SA_PASSWORD=... scripts/check-mssql-batch-results.sh 127.0.0.1 14339 sa pr_mcp_sql_server_batches, 52/52 checks pass, twice. That is the driver path this change sends every external script through, including the reporter's script as one batch with four result sets.Not covered
PostLaunchWorkreturns early when storage is isolated) and only the Settings pane starts it; AppleScript needs Automation consent. The bridge is covered by the unit tests above over a fake driver, and the server side by the live check.Found while fixing #3078 (#3105).