Fix: enable query plans on SQL Server via EXPLAIN (#331)

* Fix: enable query plans on SQL Server via EXPLAIN

SQL Server has no native EXPLAIN statement, and the bogus `explain`/
`showplan` entries in the read-only allow-list did nothing. The actual
mechanism, SET SHOWPLAN_XML ON, is session scoped, must be the only
statement in its batch, and is suppressed inside a transaction — so it
could not be used through the pooled, stateless connector.

Translate a leading `EXPLAIN <query>` into a SHOWPLAN_XML request run on
a short-lived single-connection pool, giving SQL Server a Postgres/MySQL
-like EXPLAIN. SHOWPLAN_XML compiles without executing, so it is
read-only safe; the isolated session keeps SHOWPLAN state off the shared
pool. Drop the dead `showplan` keyword.

Closes #310

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* Harden SQL Server EXPLAIN translation (PR review)

Address Copilot review feedback on the EXPLAIN/SHOWPLAN flow:

- Skip leading whitespace and SQL comments when detecting EXPLAIN, so a
  comment-prefixed EXPLAIN that passes the (comment-stripping) read-only
  validator is also translated instead of reaching the server untranslated.
- Trim the extracted inner query.
- Reject empty EXPLAIN and any SET SHOWPLAN inside the explained statement.
  SQL Server already blocks SET SHOWPLAN alongside other statements in a
  batch (verified: the DELETE does not execute), but this keeps the
  read-only guarantee self-contained rather than relying on server behavior.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* Treat comment-only EXPLAIN as empty (PR review)

Validate the explained statement against comment/string-stripped SQL so
`EXPLAIN /* comment */` raises the "requires a statement" error instead
of running an empty batch. Reuses the strip already done for the SET
SHOWPLAN guard.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Author: tianzhou

src/connectors/__tests__/sqlserver.integration.test.ts

@@ -264,6 +264,81 @@ describe('SQL Server Connector Integration Tests', () => {
   });
 
   describe('SQL Server-specific Features', () => {
+    it('should return an execution plan for EXPLAIN <query> (SHOWPLAN_XML)', async () => {
+      const result = await sqlServerTest.connector.executeSQL(
+        'EXPLAIN SELECT * FROM users WHERE age > 30',
+        {}
+      );
+
+      expect(result.rows).toHaveLength(1);
+      const planXml = result.rows[0].plan as string;
+      expect(typeof planXml).toBe('string');
+      // SHOWPLAN_XML output is a ShowPlanXML document referencing the query.
+      expect(planXml).toContain('ShowPlanXML');
+      expect(planXml).toContain('users');
+    });
+
+    it('should not execute the statement under EXPLAIN', async () => {
+      const before = await sqlServerTest.connector.executeSQL(
+        "SELECT COUNT(*) as count FROM users WHERE email = 'explain-noexec@example.com'",
+        {}
+      );
+      expect(Number(before.rows[0].count)).toBe(0);
+
+      // SHOWPLAN_XML compiles without executing, so this INSERT must not run.
+      await sqlServerTest.connector.executeSQL(
+        "EXPLAIN INSERT INTO users (name, email, age) VALUES ('NoExec', 'explain-noexec@example.com', 99)",
+        {}
+      );
+
+      const after = await sqlServerTest.connector.executeSQL(
+        "SELECT COUNT(*) as count FROM users WHERE email = 'explain-noexec@example.com'",
+        {}
+      );
+      expect(Number(after.rows[0].count)).toBe(0);
+    });
+
+    it('should translate EXPLAIN even when preceded by a comment', async () => {
+      const result = await sqlServerTest.connector.executeSQL(
+        '/* inspect plan */ EXPLAIN SELECT * FROM users',
+        {}
+      );
+      expect(result.rows).toHaveLength(1);
+      expect(result.rows[0].plan as string).toContain('ShowPlanXML');
+    });
+
+    it('should reject SET SHOWPLAN smuggled into an EXPLAIN query', async () => {
+      const before = await sqlServerTest.connector.executeSQL(
+        'SELECT COUNT(*) as count FROM users',
+        {}
+      );
+
+      await expect(
+        sqlServerTest.connector.executeSQL(
+          'EXPLAIN SET SHOWPLAN_XML OFF DELETE FROM users',
+          {}
+        )
+      ).rejects.toThrow(/SET SHOWPLAN/i);
+
+      const after = await sqlServerTest.connector.executeSQL(
+        'SELECT COUNT(*) as count FROM users',
+        {}
+      );
+      expect(Number(after.rows[0].count)).toBe(Number(before.rows[0].count));
+    });
+
+    it('should reject an empty EXPLAIN', async () => {
+      await expect(
+        sqlServerTest.connector.executeSQL('EXPLAIN   ', {})
+      ).rejects.toThrow(/requires a statement/i);
+    });
+
+    it('should reject a comment-only EXPLAIN', async () => {
+      await expect(
+        sqlServerTest.connector.executeSQL('EXPLAIN /* just a comment */', {})
+      ).rejects.toThrow(/requires a statement/i);
+    });
+
     it('should handle SQL Server IDENTITY columns', async () => {
       await sqlServerTest.connector.executeSQL(`
         CREATE TABLE identity_test (

src/connectors/sqlserver/index.ts

@@ -16,6 +16,7 @@ import { isDriverNotInstalled } from "../../utils/module-loader.js";
 import { SafeURL } from "../../utils/safe-url.js";
 import { obfuscateDSNPassword } from "../../utils/dsn-obfuscate.js";
 import { SQLRowLimiter } from "../../utils/sql-row-limiter.js";
+import { stripCommentsAndStrings } from "../../utils/sql-parser.js";
 
 /**
  * SQL Server DSN parser
@@ -172,6 +173,14 @@ export class SQLServerConnector implements Connector {
   // Source ID is set by ConnectorManager after cloning
   private sourceId: string = "default";
 
+  /**
+   * Leading whitespace and SQL comments to skip before looking for a keyword.
+   * The read-only validator strips comments before checking the first keyword,
+   * so the connector must skip them too; otherwise an EXPLAIN preceded by a
+   * comment passes validation but reaches the server untranslated.
+   */
+  private static readonly LEADING_NOISE = /^(?:\s+|--[^\n]*(?:\n|$)|\/\*[\s\S]*?\*\/)*/;
+
   getId(): string {
     return this.sourceId;
   }
@@ -602,6 +611,17 @@ export class SQLServerConnector implements Connector {
       throw new Error("Not connected to SQL Server database");
     }
 
+    // SQL Server has no native EXPLAIN statement. Translate a leading `EXPLAIN`
+    // into a SHOWPLAN_XML request so callers get a Postgres/MySQL-like
+    // experience. SHOWPLAN_XML compiles the statement without executing it, so
+    // this is read-only safe (further enforced in explainQuery).
+    const afterNoise = sqlQuery.slice(
+      sqlQuery.match(SQLServerConnector.LEADING_NOISE)![0].length
+    );
+    if (/^explain\b/i.test(afterNoise)) {
+      return this.explainQuery(afterNoise.slice("explain".length).trim());
+    }
+
     try {
       // Apply maxRows limit to SELECT queries if specified
       let processedSQL = sqlQuery;
@@ -673,6 +693,65 @@ export class SQLServerConnector implements Connector {
       throw new Error(`Failed to execute query: ${(error as Error).message}`);
     }
   }
+
+  /**
+   * Return the estimated execution plan for a query using SHOWPLAN_XML.
+   *
+   * SHOWPLAN_XML compiles the statement and returns its plan without executing
+   * it, but it has two constraints: `SET SHOWPLAN_XML ON` must be the only
+   * statement in its batch, and the setting is session scoped. The shared pool
+   * hands out a fresh connection per request() and an open transaction
+   * suppresses SHOWPLAN, so neither can carry the setting to a follow-up query.
+   *
+   * We therefore run the SET / query pair on a short-lived, single-connection
+   * pool built from the same config. The dedicated session keeps SHOWPLAN state
+   * off the shared pool, so a concurrent query can never land on a connection
+   * with SHOWPLAN enabled (which would return a plan instead of its results).
+   */
+  private async explainQuery(innerQuery: string): Promise<SQLResult> {
+    // Validate against comment/string-stripped SQL so comment-only input counts
+    // as empty and a SET SHOWPLAN can't hide behind comments.
+    const cleaned = stripCommentsAndStrings(innerQuery, "sqlserver").trim();
+    if (!cleaned) {
+      throw new Error("EXPLAIN requires a statement to analyze");
+    }
+
+    // Defense in depth: the SET SHOWPLAN session toggle is what makes EXPLAIN
+    // non-executing, so the explained statement must not disable it. SQL Server
+    // already rejects `SET SHOWPLAN_* OFF` alongside other statements in a
+    // batch, but enforcing it here keeps the read-only guarantee self-contained.
+    if (/\bset\s+showplan/i.test(cleaned)) {
+      throw new Error("EXPLAIN does not support SET SHOWPLAN statements");
+    }
+
+    if (!this.config) {
+      throw new Error("Not connected to SQL Server database");
+    }
+
+    const explainPool = new sql.ConnectionPool({
+      ...this.config,
+      pool: { ...this.config.pool, max: 1, min: 1 },
+    });
+
+    try {
+      await explainPool.connect();
+      // max:1 + sequential awaits guarantee both batches hit the same session.
+      await explainPool.request().batch("SET SHOWPLAN_XML ON");
+      const planResult = await explainPool.request().batch(innerQuery);
+
+      // The plan is returned as the single column of the first row.
+      const planRow = planResult.recordset?.[0];
+      const planXml = planRow ? Object.values(planRow)[0] : null;
+      return {
+        rows: planXml != null ? [{ plan: planXml }] : [],
+        rowCount: planXml != null ? 1 : 0,
+      };
+    } catch (error) {
+      throw new Error(`Failed to explain query: ${(error as Error).message}`);
+    } finally {
+      await explainPool.close();
+    }
+  }
 }
 
 // Create and register the connector

src/utils/__tests__/allowed-keywords.test.ts

@@ -220,6 +220,20 @@ describe("isReadOnlySQL", () => {
     });
   });
 
+  describe("SQL Server keywords", () => {
+    it("should allow EXPLAIN (translated to SHOWPLAN_XML by the connector)", () => {
+      expect(isReadOnlySQL("EXPLAIN SELECT * FROM users", "sqlserver")).toBe(true);
+    });
+
+    it("should reject SHOWPLAN (not a real T-SQL statement)", () => {
+      expect(isReadOnlySQL("SHOWPLAN SELECT * FROM users", "sqlserver")).toBe(false);
+    });
+
+    it("should reject bare SET SHOWPLAN_XML (session-scoped, handled via EXPLAIN)", () => {
+      expect(isReadOnlySQL("SET SHOWPLAN_XML ON", "sqlserver")).toBe(false);
+    });
+  });
+
   describe("edge cases", () => {
     it("should treat empty SQL after comment stripping as not read-only", () => {
       expect(isReadOnlySQL("-- just a comment", "postgres")).toBe(false);

src/utils/allowed-keywords.ts

@@ -11,7 +11,9 @@ export const allowedKeywords: Record<ConnectorType, string[]> = {
   mysql: ["select", "with", "explain", "show", "describe", "desc"],
   mariadb: ["select", "with", "explain", "show", "describe", "desc"],
   sqlite: ["select", "with", "explain", "pragma"],
-  sqlserver: ["select", "with", "explain", "showplan"],
+  // SQL Server has no native EXPLAIN statement; the connector translates a
+  // leading `EXPLAIN` into a SET SHOWPLAN_XML request (see SQLServerConnector).
+  sqlserver: ["select", "with", "explain"],
 };
 
 /**