[VibeSWE]

Originally stacked on #62. #62 has merged, and this PR now targets main.

Fixes review follow-up 19 from #62. The code is upstream-inherited, from parseablehq/parseable 32979cb.

Problem

bounded_tool_sql wraps the caller's query as SELECT * FROM (<query>\n) AS "__parseable_tool_result" LIMIT 500. It removes the statement terminator with query.trim().trim_end_matches(';'), which only works when the text ends in ;. DataFusion's parser accepts SELECT a FROM t; -- note as one statement, so validation passes. But the ; is not the last character, so it stays inside the wrapper:

SELECT * FROM (SELECT 'x' AS a; -- note
) AS "__parseable_tool_result" LIMIT 500
→ SQL error: ParserError("Expected: ), found: AS at Line: 1, Column: 27")

So query_sql and explain_query return a confusing runtime parse error for a query the validator accepted. The same happens with ; /* note */ and ; ;.

Fix

A new helper, strip_trailing_semicolons, tokenizes the query with sqlparser's tokenizer, using GenericDialect, the same dialect DFParser uses. It finds the trailing run of ; and whitespace or comment tokens and cuts the original text at the first ; in that run. Otherwise the caller's SQL is passed through verbatim. A ; inside a string literal or a comment is never treated as the terminator. A trailing comment with no semicolon is still preserved, as the existing upstream test expects.

I didn't take the alternative of rebuilding the SQL from the AST (statement.to_string()). It re-renders the whole query, so formatting, quoting and literal spelling can drift from what the caller wrote and what DataFusion would plan. Cutting at a token boundary keeps the change minimal.

Projection checks inside CTEs and subqueries (optional part, not done)

validate_projection still checks only the outermost query body: parenthesized queries and each branch of a set operation. It does not descend into CTEs, derived tables or subquery expressions. Extending it would reject legitimate SQL such as WHERE EXISTS (SELECT * …) and WITH x AS (SELECT * FROM t) SELECT a, b FROM x. Such queries can come from agents reusing dashboard tile SQL. Meanwhile, the result width is already bounded by the outer projection check: at most 10 explicit items and no *, in every branch of a set operation. The fork's generated queries (list_log_errors_query, trace_activity_query, sample_events_query) are flat and unaffected either way. New assertions pin down that behaviour, which docs/tool-api.md on #62 already describes accurately.

Docs

#62's "SQL tool limits" section documented the semicolon-plus-comment failure as a known limitation. This PR replaces that sentence with the fixed behaviour: trailing semicolons and comments are allowed, including comments after the final semicolon.

Tests

• New: bounded_tool_sql_runs_with_comments_after_the_trailing_semicolon executes the wrapped SQL in a real DataFusion SessionContext for eight cases: ; -- c, ; /* c */, ; ; followed by comments, a comment before ;, ';' inside a string literal, ; inside a line comment, ; inside a block comment, and a non-ASCII literal (column-to-byte offset mapping). Fails without the fix with the ParserError above.
• Extended: shared_sql_limits_reject_wildcards_and_excess_columns now asserts that * inside a CTE, derived table or EXISTS is accepted and that an outer SELECT * over a CTE is rejected.
• cargo test --lib tool_ passed, on both the branch and the merge with the latest base. cargo clippy -- -D warnings is clean. No new --all-targets warnings in touched files.

Upstream fidelity

The change touches upstream-owned code in src/tool_executor.rs: one added helper, one changed line in bounded_tool_sql, two new imports, and tests. It's small and self-contained, so future upstream merges should conflict only if upstream rewrites bounded_tool_sql itself. The same fix is worth proposing upstream.

🤖 Generated with Claude Code