fix: address 7 P1 findings from v0.5.16 release evaluation (#590)

ClickHouse driver:
- Silently ignore `binds` param (was throwing, inconsistent with other drivers)
- Strip SQL comments before LIMIT check to prevent `-- LIMIT` bypass
- Remove redundant `hasDML` regex (dead code after `isDDL` routing)
- Replace fragile `position(type, 'Nullable')` SQL with TS regex on type string
- Add connection guard: `execute()` before `connect()` throws clear error

Query history:
- Rename `{days:UInt32}` placeholders to `__DAYS__`/`__LIMIT__` to avoid
  confusion with ClickHouse native query parameter syntax

Docs:
- Update warehouse count from 10 to 12

Tests:
- Add 39 unit tests for ClickHouse driver (DDL routing, LIMIT injection,
  truncation, nullable, connection lifecycle, binds, column mapping)
- Remove stale "ClickHouse unsupported" test

Author: anandgupta42

docs/docs/configure/warehouses.md

@@ -1,6 +1,6 @@
 # Warehouses
 
-Altimate Code connects to 10 warehouse types. Configure them in `.altimate-code/connections.json` (project-local) or `~/.altimate-code/connections.json` (global).
+Altimate Code connects to 12 warehouse types. Configure them in `.altimate-code/connections.json` (project-local) or `~/.altimate-code/connections.json` (global).
 
 ## Configuration
 

packages/drivers/src/clickhouse.ts

@@ -60,17 +60,16 @@ export async function connect(config: ConnectionConfig): Promise<Connector> {
       client = createClient(clientConfig)
     },
 
-    async execute(sql: string, limit?: number, binds?: any[]): Promise<ConnectorResult> {
-      if (binds && binds.length > 0) {
-        throw new Error("ClickHouse driver does not support parameterized binds — use ClickHouse query parameters instead")
+    async execute(sql: string, limit?: number, _binds?: any[]): Promise<ConnectorResult> {
+      if (!client) {
+        throw new Error("ClickHouse client not connected — call connect() first")
       }
       const effectiveLimit = limit === undefined ? 1000 : limit
       let query = sql
       // Only SELECT and WITH...SELECT support LIMIT — SHOW/DESCRIBE/EXPLAIN/EXISTS do not
       const supportsLimit = /^\s*(SELECT|WITH)\b/i.test(sql)
       const isDDL =
         /^\s*(INSERT|CREATE|DROP|ALTER|TRUNCATE|RENAME|ATTACH|DETACH|OPTIMIZE|SYSTEM|SET|USE|GRANT|REVOKE)\b/i.test(sql)
-      const hasDML = /\b(INSERT|CREATE|DROP|ALTER|TRUNCATE|RENAME|ATTACH|DETACH|OPTIMIZE|SYSTEM)\b/i.test(sql)
 
       // DDL/DML: use client.command() — no result set expected
       if (isDDL) {
@@ -79,8 +78,10 @@ export async function connect(config: ConnectionConfig): Promise<Connector> {
       }
 
       // Read queries: use client.query() with JSONEachRow format
-      // Only append LIMIT for SELECT/WITH queries (not SHOW/DESCRIBE/EXPLAIN/EXISTS)
-      if (supportsLimit && !hasDML && effectiveLimit > 0 && !/\bLIMIT\b/i.test(sql)) {
+      // Only append LIMIT for SELECT/WITH queries that don't already have one.
+      // Strip SQL comments before checking for LIMIT to prevent bypass via `-- LIMIT`.
+      const sqlNoComments = sql.replace(/--[^\n]*/g, "").replace(/\/\*[\s\S]*?\*\//g, "")
+      if (supportsLimit && effectiveLimit > 0 && !/\bLIMIT\b/i.test(sqlNoComments)) {
         query = `${sql.replace(/;\s*$/, "")} LIMIT ${effectiveLimit + 1}`
       }
 
@@ -134,8 +135,7 @@ export async function connect(config: ConnectionConfig): Promise<Connector> {
 
     async describeTable(schema: string, table: string): Promise<SchemaColumn[]> {
       const resultSet = await client.query({
-        query: `SELECT name, type,
-                       position(type, 'Nullable') > 0 AS is_nullable
+        query: `SELECT name, type
                 FROM system.columns
                 WHERE database = {db:String}
                   AND table = {tbl:String}
@@ -147,7 +147,8 @@ export async function connect(config: ConnectionConfig): Promise<Connector> {
       return rows.map((r) => ({
         name: r.name as string,
         data_type: r.type as string,
-        nullable: r.is_nullable === 1 || r.is_nullable === true || r.is_nullable === "1",
+        // Detect Nullable from the type string directly — stable across all versions
+        nullable: /^Nullable\b/i.test((r.type as string) ?? ""),
       }))
     },
 

packages/drivers/test/clickhouse-unit.test.ts

@@ -0,0 +1,319 @@
+/**
+ * Unit tests for ClickHouse driver logic:
+ * - DDL vs SELECT routing (command vs query)
+ * - LIMIT injection and bypass prevention
+ * - Truncation detection
+ * - Nullable detection from type string
+ * - Connection guard (execute before connect)
+ * - Binds parameter is silently ignored
+ */
+import { describe, test, expect, mock, beforeEach } from "bun:test"
+
+// --- Mock @clickhouse/client ---
+
+let mockCommandCalls: any[] = []
+let mockQueryCalls: any[] = []
+let mockQueryResult: any[] = []
+let mockCloseCalls = 0
+
+function resetMocks() {
+  mockCommandCalls = []
+  mockQueryCalls = []
+  mockQueryResult = []
+  mockCloseCalls = 0
+}
+
+mock.module("@clickhouse/client", () => ({
+  createClient: (_config: any) => ({
+    command: async (opts: any) => {
+      mockCommandCalls.push(opts)
+    },
+    query: async (opts: any) => {
+      mockQueryCalls.push(opts)
+      return { json: async () => mockQueryResult }
+    },
+    close: async () => {
+      mockCloseCalls++
+    },
+  }),
+}))
+
+// Import after mocking
+const { connect } = await import("../src/clickhouse")
+
+describe("ClickHouse driver unit tests", () => {
+  let connector: Awaited<ReturnType<typeof connect>>
+
+  beforeEach(async () => {
+    resetMocks()
+    connector = await connect({ host: "localhost", port: 8123 })
+    await connector.connect()
+  })
+
+  // --- DDL vs SELECT routing ---
+
+  describe("DDL routing via client.command()", () => {
+    const ddlStatements = [
+      "INSERT INTO t VALUES (1, 'a')",
+      "CREATE TABLE t (id UInt32) ENGINE = MergeTree()",
+      "DROP TABLE t",
+      "ALTER TABLE t ADD COLUMN x String",
+      "TRUNCATE TABLE t",
+      "OPTIMIZE TABLE t FINAL",
+      "SYSTEM RELOAD DICTIONARY",
+      "SET max_memory_usage = 1000000",
+    ]
+
+    for (const sql of ddlStatements) {
+      test(`routes "${sql.slice(0, 40)}..." to client.command()`, async () => {
+        const result = await connector.execute(sql)
+        expect(mockCommandCalls.length).toBe(1)
+        expect(mockQueryCalls.length).toBe(0)
+        expect(result.row_count).toBe(0)
+      })
+    }
+
+    test("strips trailing semicolons from DDL", async () => {
+      await connector.execute("DROP TABLE t;  ")
+      expect(mockCommandCalls[0].query).toBe("DROP TABLE t")
+    })
+  })
+
+  describe("SELECT routing via client.query()", () => {
+    test("routes SELECT to client.query()", async () => {
+      mockQueryResult = [{ id: 1, name: "test" }]
+      await connector.execute("SELECT id, name FROM t")
+      expect(mockQueryCalls.length).toBe(1)
+      expect(mockCommandCalls.length).toBe(0)
+    })
+
+    test("routes SHOW to client.query()", async () => {
+      mockQueryResult = [{ name: "db1" }]
+      await connector.execute("SHOW DATABASES")
+      expect(mockQueryCalls.length).toBe(1)
+    })
+
+    test("routes DESCRIBE to client.query()", async () => {
+      mockQueryResult = [{ name: "col1", type: "String" }]
+      await connector.execute("DESCRIBE TABLE t")
+      expect(mockQueryCalls.length).toBe(1)
+    })
+
+    test("routes EXPLAIN to client.query()", async () => {
+      mockQueryResult = [{ explain: "ReadFromMergeTree" }]
+      await connector.execute("EXPLAIN SELECT 1")
+      expect(mockQueryCalls.length).toBe(1)
+    })
+  })
+
+  // --- LIMIT injection ---
+
+  describe("LIMIT injection", () => {
+    test("appends LIMIT to SELECT without one", async () => {
+      mockQueryResult = [{ id: 1 }]
+      await connector.execute("SELECT * FROM t", 10)
+      expect(mockQueryCalls[0].query).toContain("LIMIT 11")
+    })
+
+    test("does NOT append LIMIT to SELECT that already has one", async () => {
+      mockQueryResult = [{ id: 1 }]
+      await connector.execute("SELECT * FROM t LIMIT 5", 10)
+      expect(mockQueryCalls[0].query).not.toContain("LIMIT 11")
+    })
+
+    test("does NOT append LIMIT to SHOW/DESCRIBE/EXPLAIN/EXISTS", async () => {
+      mockQueryResult = [{ name: "t" }]
+
+      await connector.execute("SHOW TABLES", 10)
+      expect(mockQueryCalls[0].query).not.toContain("LIMIT")
+
+      mockQueryCalls = []
+      await connector.execute("DESCRIBE TABLE t", 10)
+      expect(mockQueryCalls[0].query).not.toContain("LIMIT")
+
+      mockQueryCalls = []
+      await connector.execute("EXISTS TABLE t", 10)
+      expect(mockQueryCalls[0].query).not.toContain("LIMIT")
+    })
+
+    test("does NOT append LIMIT when limit=0 (unlimited)", async () => {
+      mockQueryResult = [{ id: 1 }, { id: 2 }]
+      await connector.execute("SELECT * FROM t", 0)
+      expect(mockQueryCalls[0].query).not.toContain("LIMIT")
+    })
+
+    test("uses default limit=1000 when limit is undefined", async () => {
+      mockQueryResult = [{ id: 1 }]
+      await connector.execute("SELECT * FROM t")
+      expect(mockQueryCalls[0].query).toContain("LIMIT 1001")
+    })
+
+    test("LIMIT in SQL comment does NOT prevent LIMIT injection", async () => {
+      mockQueryResult = [{ id: 1 }]
+      await connector.execute("SELECT * FROM t -- LIMIT 100", 10)
+      // Should still append LIMIT because the comment-stripped SQL has no LIMIT
+      expect(mockQueryCalls[0].query).toContain("LIMIT 11")
+    })
+
+    test("LIMIT in block comment does NOT prevent LIMIT injection", async () => {
+      mockQueryResult = [{ id: 1 }]
+      await connector.execute("SELECT * FROM t /* LIMIT 50 */", 10)
+      expect(mockQueryCalls[0].query).toContain("LIMIT 11")
+    })
+
+    test("real LIMIT in SQL still prevents double LIMIT", async () => {
+      mockQueryResult = [{ id: 1 }]
+      await connector.execute("SELECT * FROM t LIMIT 5 -- max rows", 10)
+      expect(mockQueryCalls[0].query).not.toContain("LIMIT 11")
+    })
+  })
+
+  // --- Truncation detection ---
+
+  describe("truncation detection", () => {
+    test("detects truncation when rows exceed limit", async () => {
+      mockQueryResult = Array.from({ length: 6 }, (_, i) => ({ id: i }))
+      const result = await connector.execute("SELECT * FROM t", 5)
+      expect(result.truncated).toBe(true)
+      expect(result.row_count).toBe(5)
+      expect(result.rows.length).toBe(5)
+    })
+
+    test("no truncation when rows equal limit", async () => {
+      mockQueryResult = Array.from({ length: 5 }, (_, i) => ({ id: i }))
+      const result = await connector.execute("SELECT * FROM t", 5)
+      expect(result.truncated).toBe(false)
+      expect(result.row_count).toBe(5)
+    })
+
+    test("no truncation when rows below limit", async () => {
+      mockQueryResult = [{ id: 1 }]
+      const result = await connector.execute("SELECT * FROM t", 10)
+      expect(result.truncated).toBe(false)
+      expect(result.row_count).toBe(1)
+    })
+
+    test("limit=0 returns all rows without truncation", async () => {
+      mockQueryResult = Array.from({ length: 100 }, (_, i) => ({ id: i }))
+      const result = await connector.execute("SELECT * FROM t", 0)
+      expect(result.truncated).toBe(false)
+      expect(result.row_count).toBe(100)
+    })
+
+    test("empty result returns correctly", async () => {
+      mockQueryResult = []
+      const result = await connector.execute("SELECT * FROM t", 10)
+      expect(result.row_count).toBe(0)
+      expect(result.columns).toEqual([])
+      expect(result.truncated).toBe(false)
+    })
+  })
+
+  // --- Nullable detection ---
+
+  describe("describeTable nullable detection", () => {
+    test("detects Nullable(String) as nullable", async () => {
+      mockQueryResult = [{ name: "col1", type: "Nullable(String)" }]
+      const cols = await connector.describeTable("default", "t")
+      expect(cols[0].nullable).toBe(true)
+    })
+
+    test("detects String as non-nullable", async () => {
+      mockQueryResult = [{ name: "col1", type: "String" }]
+      const cols = await connector.describeTable("default", "t")
+      expect(cols[0].nullable).toBe(false)
+    })
+
+    test("detects Nullable(UInt32) as nullable", async () => {
+      mockQueryResult = [{ name: "col1", type: "Nullable(UInt32)" }]
+      const cols = await connector.describeTable("default", "t")
+      expect(cols[0].nullable).toBe(true)
+    })
+
+    test("Array(Nullable(String)) is NOT nullable at column level", async () => {
+      // The column itself isn't Nullable — the array elements are
+      mockQueryResult = [{ name: "col1", type: "Array(Nullable(String))" }]
+      const cols = await connector.describeTable("default", "t")
+      expect(cols[0].nullable).toBe(false)
+    })
+
+    test("LowCardinality(Nullable(String)) is NOT nullable at column level", async () => {
+      // Nullable is nested inside LowCardinality — column-level is LowCardinality
+      mockQueryResult = [{ name: "col1", type: "LowCardinality(Nullable(String))" }]
+      const cols = await connector.describeTable("default", "t")
+      expect(cols[0].nullable).toBe(false)
+    })
+  })
+
+  // --- Connection guard ---
+
+  describe("connection lifecycle", () => {
+    test("execute before connect throws clear error", async () => {
+      const freshConnector = await connect({ host: "localhost" })
+      // Don't call connect()
+      expect(freshConnector.execute("SELECT 1")).rejects.toThrow("not connected")
+    })
+
+    test("close is idempotent", async () => {
+      await connector.close()
+      await connector.close() // should not throw
+      expect(mockCloseCalls).toBe(1) // only called once
+    })
+  })
+
+  // --- Binds parameter ---
+
+  describe("binds parameter", () => {
+    test("binds parameter is silently ignored", async () => {
+      mockQueryResult = [{ id: 1 }]
+      // Should not throw — binds are ignored
+      const result = await connector.execute("SELECT 1", 10, ["unused", "binds"])
+      expect(result.row_count).toBe(1)
+    })
+
+    test("empty binds array works fine", async () => {
+      mockQueryResult = [{ id: 1 }]
+      const result = await connector.execute("SELECT 1", 10, [])
+      expect(result.row_count).toBe(1)
+    })
+  })
+
+  // --- Column mapping ---
+
+  describe("result format", () => {
+    test("maps rows to column-ordered arrays", async () => {
+      mockQueryResult = [
+        { id: 1, name: "alice", age: 30 },
+        { id: 2, name: "bob", age: 25 },
+      ]
+      const result = await connector.execute("SELECT * FROM t", 10)
+      expect(result.columns).toEqual(["id", "name", "age"])
+      expect(result.rows).toEqual([
+        [1, "alice", 30],
+        [2, "bob", 25],
+      ])
+    })
+  })
+
+  // --- listTables type detection ---
+
+  describe("listTables engine-to-type mapping", () => {
+    test("MergeTree engines map to table", async () => {
+      mockQueryResult = [{ name: "t1", engine: "MergeTree" }]
+      const tables = await connector.listTables("default")
+      expect(tables[0].type).toBe("table")
+    })
+
+    test("MaterializedView maps to view", async () => {
+      mockQueryResult = [{ name: "v1", engine: "MaterializedView" }]
+      const tables = await connector.listTables("default")
+      expect(tables[0].type).toBe("view")
+    })
+
+    test("View maps to view", async () => {
+      mockQueryResult = [{ name: "v2", engine: "View" }]
+      const tables = await connector.listTables("default")
+      expect(tables[0].type).toBe("view")
+    })
+  })
+})

packages/drivers/test/driver-security.test.ts

@@ -623,16 +623,7 @@ describe("Connection registry", () => {
       reset()
     })
 
-    test("ClickHouse gives helpful hint", async () => {
-      setConfigs({ ch: { type: "clickhouse" } as any })
-      try {
-        await get("ch")
-        expect.unreachable("Should have thrown")
-      } catch (e: any) {
-        expect(e.message).toContain("ClickHouse is not yet supported")
-        expect(e.message).toContain("clickhouse-client")
-      }
-    })
+    // ClickHouse test removed — ClickHouse is now a supported driver (v0.5.16)
 
     test("Cassandra gives helpful hint", async () => {
       setConfigs({ cass: { type: "cassandra" } as any })