fix(editor): treat invisible characters around a statement as blank on the run path - #2729
Merged
Merged
Conversation
…n the run path Claude-Session: https://claude.ai/code/session_011THKc9TRHE8xjcxXidDHXP
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Automations to automatically generate PRs for you. |
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.
Found while investigating #2717 (PR #2724).
Root cause
The run path used three different definitions of whitespace, and they disagreed:
contentRangeonly knew space, tab, LF and CR.CharacterSet.whitespacesAndNewlines, which includes U+00A0, U+3000 and U+200B but not U+FEFF or U+0008.QueryClassifierskippedCharacter.isWhitespace.So a line holding only U+00A0 counted as a statement for the gutter but trimmed to nothing at run time: the run button showed and did nothing. And
"\u{0008}SELECT 1"kept its U+0008, the leading keyword came out empty, the unknown-keyword arm answered "write", and Read-Only Safe Mode refused a plainSELECT. A leading BOM hid aDROPthe same way, just in the other direction.The review turned up two more ways the classifier could miss a write, in the same comment-skipping code this change edits:
Characters to"\n". In Swift, CRLF is one"\r\n"Character, so-- note\r\nDROP TABLE usersstripped to""and classified as safe. A bare CR did the same. That text reaches the classifier through MCPexecute_queryon a read-only external connection, or through a CRLF.sqlfile under Read-Only Safe Mode./*! ... */and MariaDB/*M! ... */comments, which the server executes, so/*!50000 DROP TABLE users */also classified as safe.What changed
StatementBlank(TablePro/Core/Utilities/SQL/StatementBlank.swift) is the one definition of blank for the run path. A character is blank when every scalar in it is Unicode whitespace, a control character, a default-ignorable code point or U+FFF9 to U+FFFB, and is not a letter. ASCII skips the property lookups.❤️, flag tag characters, a ZWJ after a virama, a ZWNJ ending a Persian word. An invisible character inside a word is never removed, soSEL\u{200B}ECT 1still reaches the server as written and stays a write.statementAtCursor,allStatementsPreservingSemicolons, the JavaScript scanner'shasContent, the classifier's comment and keyword skipping, andDatabaseAccessBridge(MCP and AppleScript). The text an external client has classified is the text that is sent..whitespacesAndNewlinesguards inQueryExecutionCoordinatorandMainContentCoordinatorare gone. The executable-statement list is the only gate, so the gutter's run controls and execution agree by construction.QueryClassifier: a line comment ends at LF, CR or CRLF in all three places that skip comments (strippingLeadingComments,strippingStringLiterals, theEXPLAINinner-statement walk). A statement holding a conditional comment is never safe: at least a write, destructive when the revealed text hasDROPorTRUNCATE, and flagged when it touches the filesystem or runs code. An opener inside a string literal or an ordinary comment stays inert.DatabaseAccessBridge.runStatementrefuses a statement that trims to nothing ("The query is empty.", already in the catalog) before it opens a connection.docs/features/sql-editor.mdxsays invisible characters around a statement are not sent and a line of them gets no run button.docs/features/safe-mode.mdxsays a conditional comment makes a statement count as a write.Verification
verify.sh buildpassed,.analysis/fix-run-path-invisible-whitespace/logs/build-TablePro-161542.log.StatementBlankTests, tenQueryClassifier*suites (including the newQueryClassifierCommentBoundaryTestsandQueryClassifierInvisibleCharacterTests),SQLExecutableStatementTests,SQLStatementNavigationTests,SQLStatementScannerTests,SQLStatementBlockSplittingTests,SQLStatementRangeTests,SQLStatementScannerLocatedTests,StatementAnchorTests,StatementNavigationCommandTests,StatementRunPerformanceGuardTests,StatementRunControllerTests,JavaScriptStatementScannerTests,QueryStatementModelTests,DatabaseAccessBridgeStatementTests,ExternalStatementGateTests,ExecutionGateTests,MCPStatementGateConsentPolicyTests,MCPStatementGateRefusalTests,ConfirmDestructiveOperationToolTests,ExportDataToolStatementTests,ExplainResultRouterTests,StringCatalogIntegrityTests. Log:.analysis/fix-run-path-invisible-whitespace/logs/test-StatementBlankTests-161621.log.test-SQLExecutableStatementTests-152352.log.QueryClassifierCommentBoundaryTests,ExternalStatementGateTests,DatabaseAccessBridgeStatementTestsandSQLExecutableStatementTests. Log:test-QueryClassifierCommentBoundaryTests-161227.log.--stricton the changed files: no violations on added lines. The two hits are on existing lines 12 and 16 ofSQLExecutableStatementTests.swift.scripts/localization.py verify: ok. Docscheck-writing-style.shandcheck-docs-against-source.py: pass.LocalPackages/CodeEditTextViewis untouched.Notes
Codex was unavailable, so an adversarial reviewer agent read the diff instead. It raised four findings:
DROPfrom the classifier (material, existing before this change). Fixed as described above.isEmpty, so a tab of only spaces or only comments already ran nothing through those controls before this change. Giving them a runnable-statement signal is a separate change to how those controls validate.Known trade-offs, all in the safe direction:
SELECT /*!40001 SQL_NO_CACHE */ * FROM tis now a write, so Read-Only Safe Mode and read-only MCP connections refuse it. The Safe Mode page says so and tells the user to remove the comment.https://claude.ai/code/session_011THKc9TRHE8xjcxXidDHXP