fix: harden SQL classification for the tool CLI and MCP server

HK
11h ago
+514-55

Issue for this PR

Follow-up to #1609 (merged), as one commit on main. It holds the classifier hardening from later review rounds of #1609, kept out of that PR after it was approved. Most of these gaps also exist on main.

Type of change

  • Bug fix
  • New feature
  • Refactor / code improvement
  • Documentation

What does this PR do?

#1609 exposes the SQL classifier to outside agents (MCP server, altimate tool run): reads run by default, writes need --allow-write, and DROP DATABASE / DROP SCHEMA / TRUNCATE are refused even then. This PR fixes cases where the classifier read the SQL differently from the warehouse.

sql_explain: newly reachable through #1609; its tool code is unchanged from main.

  • It sent the caller's SQL to the warehouse unchecked. On Redshift and DuckDB the driver runs every statement in the text, so SELECT 1; DELETE FROM t ran the DELETE without --allow-write.
  • Input starting with ANALYZE became EXPLAIN ANALYZE, which executes the statement.
  • It now explains a single statement only, refuses EXPLAIN options in its input, and refuses blocked SQL.

Writes that were read as reads (on main too):

  • Snowflake ->> statement chains.
  • dbt Jinja anywhere in the SQL on the dbt route. {% do run_query(...) %} runs at compile time, even inside a comment.
  • Spark/Hive TRANSFORM ... USING.
  • Functions that write or call outside systems:
    • ClickHouse jdbc
    • Spark reflect and java_method
    • Databricks http_request and remote_query
    • DuckDB bigquery_execute, ducklake_*, postgres_attach and sqlite_attach
    • Snowflake GETNEXTVAL
  • Sequence advances (.NEXTVAL), including quoted or commented forms, in Oracle, Snowflake and MariaDB.
  • A quote inside a Spark or MySQL /*+ hint */, Oracle q'...' quotes, a Spark -- comment ending in a backslash, and a /* inside a comment where nesting is unknown. These now make the SQL uncertain, so it is refused rather than read.
  • ClickHouse $tag$...$tag$ heredocs are lexed as strings.

DROP DATABASE / DROP SCHEMA / TRUNCATE block (on main too). The block reads the SQL text, so it is best effort. docs/docs/usage/tool.md now says so, and recommends a database user without DROP/TRUNCATE for a hard guarantee. Within that:

  • A write's literals are read as SQL, with escapes decoded as each engine decodes them. This covers EXEC(...), sp_executesql, EXECUTE IMMEDIATE, PREPARE ... FROM, DO $$...$$, dblink_exec, and routine bodies created and called in one call.

  • TRUNCATE followed by a target is a statement wherever it sits:

    • at the start of a statement, a block (BEGIN [NOT] ATOMIC, REPEAT, ...) or a handler;
    • after THEN/ELSE, EXPLAIN (...) and PREPARE p AS;
    • in CREATE TASK ... AS TRUNCATE t and in a single-statement routine body;
    • in ALTER TABLE ... TRUNCATE [SUB]PARTITION;
    • with quoted, dotted, db..t or IF EXISTS targets, and with NOWAIT/WAIT n.

    A column named truncate stays a read when what follows it is a clause word, an operator, , or ), or when it is qualified (t.truncate).

  • DROP CATALOG, DROP NAMESPACE, DROP EXTERNAL SCHEMA and DROP PLUGGABLE DATABASE are blocked.

Regressions from #1609 fixed here. Each of these refused a valid read:

  • A column or alias named truncate, which was hard-blocked.
  • SELECT 1 AS throw on SQL Server.
  • A column named nextval.

Two consistency fixes:

  • data_diff reads an empty target_warehouse as the source warehouse in both layers.
  • The omitted-warehouse dbt dialect check follows the dbt adapter the daemon has already created.

Known limits (each refuses rather than lets through):

  • A write whose literal mentions DROP followed by DATABASE/SCHEMA/CATALOG/NAMESPACE, or the word TRUNCATE, is refused even with --allow-write. Example: INSERT INTO audit VALUES ('TRUNCATE').
  • Privilege and trigger names (GRANT TRUNCATE, BEFORE TRUNCATE) are refused.
  • PostgreSQL literals ending in a backslash are refused, since standard_conforming_strings may be off.
  • Oracle q'...' quotes are refused.
  • ClickHouse jdbc(...) is a write, so reads through it need --allow-write.
  • A table or column named truncate followed by a word other than a clause word or operator is refused (SELECT truncate x, FROM truncate t). A TRUNCATE target that is itself one of those clause words (TRUNCATE nulls) is not caught.

Follow-up, pre-existing on main: a ClickHouse quoted identifier ending in a backslash can hide a second statement from the classifier. The ClickHouse driver sends one statement per request, so that statement does not run.

Out of scope, a follow-up: enforcing the DROP/TRUNCATE block in the database, through a restricted role or a read-only transaction where the driver supports one. That would cover statements built at run time, which no text check can see.

How did you verify your code works?

  • bun run typecheck is clean.
  • bun test test/altimate/tools test/altimate/tool-cli test/altimate/native test/altimate/sql-validation-adversarial.test.ts test/altimate/sql-query-deadline.test.ts test/altimate/dbt-first-execution.test.ts: 1841 pass, 0 fail.
  • Each case above has a test:
    • classifier cases per dialect in sql-classify.test.ts;
    • runner tests for sql_explain with a fake connector, which receives nothing;
    • sql_execute tests for dbt Jinja (including a MongoDB default) in sql-comment-safety.test.ts.
  • Large crafted inputs stay linear, with timing tests, including 400 KB of whitespace in classified SQL and 200 KB after ( in sql_explain.
  • The review fixes went through independent codex and claude reviews until claude approved and codex had no blocking findings.
  • The changes went through repeated independent reviews (codex and claude).
  • Manual end to end against a local DuckDB warehouse, on a copy of the database:
    • #1609's head: tool run sql_explain with SELECT 1; DELETE FROM orders, without --allow-write, returned a plan and deleted all 100 rows.
    • This branch (13c911cb29):
      • that input, and ANALYZE DELETE FROM orders, are refused with "Only a single statement can be explained";
      • a single-statement plan still works;
      • the table keeps its 100 rows.

Cost side effects

None. No model calls, queries or cloud resources are added; the changes only refuse more SQL before it runs.

Screenshots / recordings

Not a UI change.

Checklist

  • I have tested my changes locally
  • I have not included unrelated changes in this PR
  • I checked for cost side effects (model calls, queries or cloud resources that grow with user data) and described them, or wrote "none"

🤖 Generated with Claude Code

Summary by cubic

Hardens SQL classification for the tool CLI and MCP server so destructive statements are refused before they run, and valid reads stop getting blocked. The hard block on DROP DATABASE / DROP SCHEMA / TRUNCATE now covers more forms but is documented as best effort, since it reads SQL text: for a hard guarantee, connect with a database user lacking those privileges.

Bug Fixes

  • sql_explain now explains a single statement per call and refuses EXPLAIN options in its input, since some drivers run every statement after the prefix and EXPLAIN ANALYZE executes the statement.
  • Refuses more writes that ran as reads: Snowflake ->> chains, dbt Jinja anywhere in the SQL on the dbt route, Spark/Hive TRANSFORM ... USING, and functions that write or call external systems (ClickHouse jdbc, Spark reflect/java_method, Databricks http_request/remote_query, DuckDB bigquery_execute/ducklake_*/postgres_attach/sqlite_attach, Snowflake GETNEXTVAL). Reads through ClickHouse jdbc(...) now need --allow-write.
  • Sequence advances through quoted or commented NEXTVAL in Oracle, Snowflake, and MariaDB count as writes.
  • SQL whose text cannot be read with certainty is refused rather than read: a quote inside a Spark/MySQL /*+ hint, Oracle q'...' quotes, and /* inside a comment where nesting is unknown.
  • The hard block reads a write's literals as SQL with escapes decoded per engine, and finds TRUNCATE at statement, block, and handler starts, after THEN/ELSE with quoted, dotted, and IF EXISTS targets and NOWAIT/WAIT, and in task and routine bodies and ALTER TABLE ... TRUNCATE PARTITION. ClickHouse $tag$...$tag$ heredocs are lexed as strings. DROP CATALOG, DROP NAMESPACE, DROP EXTERNAL SCHEMA, and DROP PLUGGABLE DATABASE are blocked too.
  • Columns or aliases named truncate, throw, and nextval, and CASE ... THEN truncate, are reads again.
  • data_diff reads an empty target_warehouse as the source warehouse; the omitted-warehouse dbt dialect check follows the adapter the daemon already created.

Summary by CodeRabbit

  • New Features

    • SQL execution supports ordered batches of up to eight labeled queries, optional warehouse selection, and reuse of earlier results. Failed steps are reported individually, and later steps continue.
    • dbt adapter types can be identified from project profiles and compiled metadata.

  • Bug Fixes

    • Expanded SQL safety checks across dialects, including additional destructive statements, side-effecting functions, and potentially unsafe query patterns. SQL safety checks are best-effort; use a database account without DROP or TRUNCATE privileges for stronger protection.
    • SQL explanation requests consistently validate statement count, options, and read-only requirements.
    • Data comparisons handle unspecified target warehouses and include errors in failed results.