From 27aeb3097ecb581450c702a4800d5aed8b789106 Mon Sep 17 00:00:00 2001
From: Mathieu Picciolli <mathieu.picciolli@gmail.com>
Date: Wed, 1 Jul 2026 19:09:00 -0400
Subject: [PATCH 1/7] refactor: enforce read-only queries via SQLite PRAGMA
 instead of regex
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Replace the keyword blocklist in assertReadOnlyQuery with engine-level
enforcement: withSaveDb now runs `PRAGMA query_only = ON` on the in-memory
database, so SQLite itself rejects any write — including cases a text scan
would miss, such as a `WITH … DELETE` CTE.

The static guard keeps only what the engine can't cover: rejecting stacked
statements (which also shuts out ATTACH/DETACH) and giving a fast, friendly
error for a non-SELECT/WITH opener. This removes the regex false positives,
so legitimate reads like `SELECT REPLACE(...)` or `LIKE '%create%'` now work.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
---
 src/save-db.ts          |  2 ++
 src/tools/query-save.ts | 31 +++++++++++--------
 test/query-save.test.ts | 66 ++++++++++++++++++-----------------------
 test/save-db.test.ts    | 18 +++++++++--
 4 files changed, 65 insertions(+), 52 deletions(-)

diff --git a/src/save-db.ts b/src/save-db.ts
index 3c1a5ee..cbdae83 100644
--- a/src/save-db.ts
+++ b/src/save-db.ts
@@ -66,6 +66,8 @@ export async function withSaveDb<T extends Record<string, unknown>>(
 		const cdbBuffer = await readFile(save.path);
 		db = cdbToSql(cdbBuffer, SQL);
 
+		db.run("PRAGMA query_only = ON;");
+
 		const output = await fn(db, save);
 
 		return validResponse(output);
diff --git a/src/tools/query-save.ts b/src/tools/query-save.ts
index ea0a138..c917d0a 100644
--- a/src/tools/query-save.ts
+++ b/src/tools/query-save.ts
@@ -109,13 +109,28 @@ function explainQueryError(error: unknown): Error {
 		);
 	}
 
+	// Raised by `PRAGMA query_only = ON` when a statement tries to write.
+	if (/readonly database|not authorized/i.test(message)) {
+		return new Error(
+			"This tool is read-only — the query attempted to modify the save, which is not allowed.",
+		);
+	}
+
 	return error instanceof Error ? error : new Error(message);
 }
 
 /**
- * Reject anything that isn't a single read-only `SELECT`/`WITH` statement.
- * The save is loaded into an in-memory sql.js database (changes are never
- * written back to disk), but we still enforce read-only intent defensively.
+ * Enforce that a query is a single statement that opens as a read `SELECT`/`WITH`.
+ *
+ * Actual write protection is delegated to the SQLite engine via
+ * `PRAGMA query_only = ON` (see {@link withSaveDb}), which reliably rejects any
+ * mutating statement — including tricks a text scan would miss, such as a
+ * `WITH … DELETE` CTE. These static checks only cover what the engine can't:
+ *  - blocking stacked statements (SQLite prepares just the first one anyway, so
+ *    the extra semicolon check keeps intent explicit and errors clear), which
+ *    also shuts out `ATTACH`/`DETACH` since those can only appear as their own
+ *    statement, and
+ *  - giving a fast, friendly error for an obviously non-read opener.
  */
 export function assertReadOnlyQuery(rawQuery: string): string {
 	// Strip a single trailing semicolon, then reject any further statement
@@ -138,15 +153,5 @@ export function assertReadOnlyQuery(rawQuery: string): string {
 		);
 	}
 
-	// Defense in depth: reject statements that could mutate or attach data.
-	const forbidden =
-		/\b(insert|update|delete|drop|create|alter|replace|attach|detach|reindex|vacuum|pragma|truncate)\b/i;
-	const match = forbidden.exec(query);
-	if (match) {
-		throw new Error(
-			`Write/DDL keyword "${match[0].toUpperCase()}" is not allowed — this tool is read-only.`,
-		);
-	}
-
 	return query;
 }
diff --git a/test/query-save.test.ts b/test/query-save.test.ts
index 5ae6a9f..3fa8f29 100644
--- a/test/query-save.test.ts
+++ b/test/query-save.test.ts
@@ -46,12 +46,14 @@ describe("assertReadOnlyQuery", () => {
 			expect(assertReadOnlyQuery(q)).toBe(q);
 		});
 
-		it("rejects SELECT with a forbidden keyword inside a LIKE string (known false-positive: \\b matches across %)", () => {
-			// '\bcreate\b' matches 'create' in '%create%' because '%' is a non-word char.
-			// This is a known limitation of the regex-based guard.
-			expect(() =>
-				assertReadOnlyQuery("SELECT * FROM foo WHERE name LIKE '%create%'"),
-			).toThrowError('Write/DDL keyword "CREATE" is not allowed');
+		it("accepts a SELECT whose string literal contains a keyword like 'create'", () => {
+			const q = "SELECT * FROM foo WHERE name LIKE '%create%'";
+			expect(assertReadOnlyQuery(q)).toBe(q);
+		});
+
+		it("accepts a SELECT using the read-only REPLACE() function", () => {
+			const q = "SELECT REPLACE(name,'a','b') FROM foo";
+			expect(assertReadOnlyQuery(q)).toBe(q);
 		});
 
 		it("accepts SELECT with ORDER BY, LIMIT, GROUP BY", () => {
@@ -138,20 +140,11 @@ describe("assertReadOnlyQuery", () => {
 		});
 	});
 
-	describe("forbidden keywords inside a SELECT", () => {
-		it("rejects SELECT with inline INSERT via INSERT INTO (subquery trick)", () => {
-			expect(() =>
-				assertReadOnlyQuery("SELECT * FROM (INSERT INTO foo VALUES (1)) AS t"),
-			).toThrowError('Write/DDL keyword "INSERT" is not allowed');
-		});
-
-		it("rejects SELECT containing DROP keyword", () => {
-			expect(() => assertReadOnlyQuery("SELECT DROP FROM foo")).toThrowError(
-				'Write/DDL keyword "DROP" is not allowed',
-			);
-		});
-
-		it("rejects SELECT containing ATTACH", () => {
+	describe("ATTACH / DETACH", () => {
+		// ATTACH and DETACH can only appear as their own top-level statement, so the
+		// opener + single-statement checks already shut them out — no keyword scan
+		// needed.
+		it("rejects ATTACH stacked after a SELECT", () => {
 			expect(() =>
 				assertReadOnlyQuery("SELECT * FROM foo; ATTACH DATABASE 'x' AS y"),
 			).toThrowError("Only a single statement is allowed");
@@ -163,28 +156,27 @@ describe("assertReadOnlyQuery", () => {
 			).toThrowError("Only read-only SELECT");
 		});
 
-		it("rejects VACUUM", () => {
-			expect(() => assertReadOnlyQuery("SELECT 1 WHERE VACUUM")).toThrowError(
-				'Write/DDL keyword "VACUUM" is not allowed',
+		it("rejects DETACH as a standalone statement opener", () => {
+			expect(() => assertReadOnlyQuery("DETACH DATABASE e")).toThrowError(
+				"Only read-only SELECT",
 			);
 		});
+	});
 
-		it("rejects ALTER", () => {
-			expect(() =>
-				assertReadOnlyQuery("SELECT * FROM foo ALTER TABLE bar"),
-			).toThrowError('Write/DDL keyword "ALTER" is not allowed');
-		});
-
-		it("rejects REPLACE", () => {
-			expect(() =>
-				assertReadOnlyQuery("SELECT REPLACE(name,'a','b') FROM foo"),
-			).toThrowError('Write/DDL keyword "REPLACE" is not allowed');
+	describe("write enforcement delegated to the engine", () => {
+		// The static guard intentionally does NOT reject write keywords that appear
+		// inside an otherwise-SELECT/WITH statement; PRAGMA query_only makes SQLite
+		// reject them at execution time. These document that the guard lets them
+		// through so it doesn't produce false positives on legitimate reads.
+		it("passes a WITH … DELETE CTE through the static guard", () => {
+			// Opens with WITH, so the guard accepts it; the engine blocks the write.
+			const q = "WITH x AS (SELECT 1) DELETE FROM foo";
+			expect(assertReadOnlyQuery(q)).toBe(q);
 		});
 
-		it("rejects TRUNCATE", () => {
-			expect(() =>
-				assertReadOnlyQuery("SELECT * FROM foo TRUNCATE"),
-			).toThrowError('Write/DDL keyword "TRUNCATE" is not allowed');
+		it("passes a SELECT mentioning a write keyword through the static guard", () => {
+			const q = "SELECT * FROM foo WHERE note = 'please update'";
+			expect(assertReadOnlyQuery(q)).toBe(q);
 		});
 	});
 });
diff --git a/test/save-db.test.ts b/test/save-db.test.ts
index fe152f4..ddfdfdd 100644
--- a/test/save-db.test.ts
+++ b/test/save-db.test.ts
@@ -22,8 +22,8 @@ const cdbToSqlMock = cdbToSql as Mock;
 
 let dir: string;
 let savePath: string;
-/** A fake sql.js database; we only care that it gets closed. */
-const fakeDb = { close: vi.fn() };
+/** A fake sql.js database; we only care that it gets closed and configured. */
+const fakeDb = { close: vi.fn(), run: vi.fn() };
 
 beforeEach(async () => {
 	dir = await mkdtemp(join(tmpdir(), "pcm-save-db-"));
@@ -33,6 +33,7 @@ beforeEach(async () => {
 	cdbToSqlMock.mockReset();
 	cdbToSqlMock.mockReturnValue(fakeDb);
 	fakeDb.close.mockReset();
+	fakeDb.run.mockReset();
 });
 
 afterEach(async () => {
@@ -60,6 +61,19 @@ describe("withSaveDb", () => {
 		expect(save.path).toBe(savePath);
 	});
 
+	it("puts the database in read-only mode before running the callback", async () => {
+		const runOrder: string[] = [];
+		fakeDb.run.mockImplementation((sql: string) => runOrder.push(sql));
+
+		await withSaveDb(savePath, () => {
+			runOrder.push("callback");
+			return {};
+		});
+
+		expect(fakeDb.run).toHaveBeenCalledWith("PRAGMA query_only = ON;");
+		expect(runOrder).toEqual(["PRAGMA query_only = ON;", "callback"]);
+	});
+
 	it("supports async callbacks", async () => {
 		const result = await withSaveDb(savePath, async () => ({ async: true }));
 

From 35596b14b1973c00b75f9dd5fa1de9230394a14c Mon Sep 17 00:00:00 2001
From: Mathieu Picciolli <mathieu.picciolli@gmail.com>
Date: Wed, 1 Jul 2026 19:11:50 -0400
Subject: [PATCH 2/7] refactor: remove redundant comments in query tests for
 clarity

---
 test/query-save.test.ts | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/test/query-save.test.ts b/test/query-save.test.ts
index 3fa8f29..2db32f7 100644
--- a/test/query-save.test.ts
+++ b/test/query-save.test.ts
@@ -141,9 +141,6 @@ describe("assertReadOnlyQuery", () => {
 	});
 
 	describe("ATTACH / DETACH", () => {
-		// ATTACH and DETACH can only appear as their own top-level statement, so the
-		// opener + single-statement checks already shut them out — no keyword scan
-		// needed.
 		it("rejects ATTACH stacked after a SELECT", () => {
 			expect(() =>
 				assertReadOnlyQuery("SELECT * FROM foo; ATTACH DATABASE 'x' AS y"),
@@ -164,12 +161,7 @@ describe("assertReadOnlyQuery", () => {
 	});
 
 	describe("write enforcement delegated to the engine", () => {
-		// The static guard intentionally does NOT reject write keywords that appear
-		// inside an otherwise-SELECT/WITH statement; PRAGMA query_only makes SQLite
-		// reject them at execution time. These document that the guard lets them
-		// through so it doesn't produce false positives on legitimate reads.
 		it("passes a WITH … DELETE CTE through the static guard", () => {
-			// Opens with WITH, so the guard accepts it; the engine blocks the write.
 			const q = "WITH x AS (SELECT 1) DELETE FROM foo";
 			expect(assertReadOnlyQuery(q)).toBe(q);
 		});

From 2154b7be5e904f113fdc9b33e39f4492147abc0b Mon Sep 17 00:00:00 2001
From: Mathieu Picciolli <mathieu.picciolli@gmail.com>
Date: Wed, 1 Jul 2026 22:33:09 -0400
Subject: [PATCH 3/7] test: add unit tests for pcm_query_save tool to enforce
 read-only behavior

---
 test/tools/query-save.test.ts | 51 +++++++++++++++++++++++++++++++++++
 1 file changed, 51 insertions(+)
 create mode 100644 test/tools/query-save.test.ts

diff --git a/test/tools/query-save.test.ts b/test/tools/query-save.test.ts
new file mode 100644
index 0000000..c0f9d07
--- /dev/null
+++ b/test/tools/query-save.test.ts
@@ -0,0 +1,51 @@
+import { beforeEach, describe, expect, it } from "vitest";
+import { registerQuerySave } from "../../src/tools/query-save";
+import { saveFixtures } from "../fixtures/save.fixture";
+import { createMockMcpServer } from "../mocks/mock-mcp-server";
+import type { MockMcpServer } from "../mocks/mock-mcp-server";
+
+describe("querySave", () => {
+	let mcp: MockMcpServer;
+
+	beforeEach(() => {
+		mcp = createMockMcpServer();
+		registerQuerySave(mcp.server);
+	});
+
+	it("registers the pcm_query_save tool", () => {
+		expect(mcp.getTool("pcm_query_save")).toBeDefined();
+		expect(mcp.registerTool).toHaveBeenCalledOnce();
+	});
+
+	it.each(saveFixtures)(
+		"runs a read-only SELECT against %s",
+		async (_name, path) => {
+			const result = await mcp.callTool("pcm_query_save", {
+				savePath: path,
+				query: "SELECT COUNT(*) AS n FROM STA_race",
+			});
+
+			expect(result.isError).toBeUndefined();
+			expect(result.structuredContent).toMatchObject({ rowCount: 1 });
+		},
+	);
+
+	// A `WITH … DELETE` CTE slips past the static SELECT/WITH guard, so the write
+	// is only stopped by `PRAGMA query_only = ON` in the engine. That surfaces a
+	// "readonly database" error, which explainQueryError maps to a friendly
+	// message — this covers that branch end-to-end.
+	it.each(saveFixtures)(
+		"maps a query_only write rejection to a read-only message for %s",
+		async (_name, path) => {
+			const result = await mcp.callTool("pcm_query_save", {
+				savePath: path,
+				query: "WITH x AS (SELECT 1) DELETE FROM STA_race",
+			});
+
+			expect(result.isError).toBe(true);
+			expect(result.content[0].text).toBe(
+				"This tool is read-only — the query attempted to modify the save, which is not allowed.",
+			);
+		},
+	);
+});

From a4c0ce26272ae5ac954502bd8e5a5819c6fc08ac Mon Sep 17 00:00:00 2001
From: Mathieu Picciolli <mathieu.picciolli@gmail.com>
Date: Thu, 2 Jul 2026 07:43:20 -0400
Subject: [PATCH 4/7] test: add tests for assertReadOnlyQuery to validate query
 enforcement

---
 test/query-save.test.ts       | 174 -------------------------
 test/tools/query-save.test.ts | 230 ++++++++++++++++++++++++++++++----
 2 files changed, 204 insertions(+), 200 deletions(-)
 delete mode 100644 test/query-save.test.ts

diff --git a/test/query-save.test.ts b/test/query-save.test.ts
deleted file mode 100644
index 2db32f7..0000000
--- a/test/query-save.test.ts
+++ /dev/null
@@ -1,174 +0,0 @@
-import { describe, expect, it } from "vitest";
-import { assertReadOnlyQuery } from "../src/tools/query-save";
-
-describe("assertReadOnlyQuery", () => {
-	describe("allowed queries", () => {
-		it("accepts a simple SELECT", () => {
-			const q = "SELECT * FROM DYN_cyclist";
-			expect(assertReadOnlyQuery(q)).toBe(q);
-		});
-
-		it("accepts SELECT with a trailing semicolon (strips it)", () => {
-			expect(assertReadOnlyQuery("SELECT 1;")).toBe("SELECT 1");
-		});
-
-		it("accepts SELECT with trailing semicolon and whitespace", () => {
-			// trim() runs before the semicolon strip, so a space before ';' is preserved
-			expect(assertReadOnlyQuery("SELECT 1 ;  ")).toBe("SELECT 1 ");
-		});
-
-		it("accepts SELECT with leading/trailing whitespace", () => {
-			expect(assertReadOnlyQuery("  SELECT id FROM foo  ")).toBe(
-				"SELECT id FROM foo",
-			);
-		});
-
-		it("accepts a WITH … SELECT (CTE)", () => {
-			const q =
-				"WITH cte AS (SELECT id FROM foo) SELECT * FROM cte WHERE id > 5";
-			expect(assertReadOnlyQuery(q)).toBe(q);
-		});
-
-		it("accepts lowercase select", () => {
-			expect(assertReadOnlyQuery("select * from bar")).toBe(
-				"select * from bar",
-			);
-		});
-
-		it("accepts mixed-case SELECT", () => {
-			expect(assertReadOnlyQuery("Select id From foo")).toBe(
-				"Select id From foo",
-			);
-		});
-
-		it("accepts SELECT containing a column named 'update_date' (keyword inside identifier)", () => {
-			const q = "SELECT update_date FROM DYN_cyclist";
-			expect(assertReadOnlyQuery(q)).toBe(q);
-		});
-
-		it("accepts a SELECT whose string literal contains a keyword like 'create'", () => {
-			const q = "SELECT * FROM foo WHERE name LIKE '%create%'";
-			expect(assertReadOnlyQuery(q)).toBe(q);
-		});
-
-		it("accepts a SELECT using the read-only REPLACE() function", () => {
-			const q = "SELECT REPLACE(name,'a','b') FROM foo";
-			expect(assertReadOnlyQuery(q)).toBe(q);
-		});
-
-		it("accepts SELECT with ORDER BY, LIMIT, GROUP BY", () => {
-			const q =
-				"SELECT name, COUNT(*) AS n FROM foo GROUP BY name ORDER BY n DESC LIMIT 10";
-			expect(assertReadOnlyQuery(q)).toBe(q);
-		});
-
-		it("accepts SELECT with a JOIN", () => {
-			const q = "SELECT a.id, b.name FROM a INNER JOIN b ON a.id = b.a_id";
-			expect(assertReadOnlyQuery(q)).toBe(q);
-		});
-	});
-
-	describe("empty / blank queries", () => {
-		it("rejects an empty string", () => {
-			expect(() => assertReadOnlyQuery("")).toThrowError("Query is empty.");
-		});
-
-		it("rejects a string that is only whitespace", () => {
-			expect(() => assertReadOnlyQuery("   ")).toThrowError("Query is empty.");
-		});
-
-		it("rejects a bare semicolon", () => {
-			expect(() => assertReadOnlyQuery(";")).toThrowError("Query is empty.");
-		});
-
-		it("rejects whitespace + semicolon", () => {
-			expect(() => assertReadOnlyQuery("  ;  ")).toThrowError(
-				"Query is empty.",
-			);
-		});
-	});
-
-	describe("multiple statements", () => {
-		it("rejects two SELECT statements separated by a semicolon", () => {
-			expect(() => assertReadOnlyQuery("SELECT 1; SELECT 2")).toThrowError(
-				"Only a single statement is allowed",
-			);
-		});
-
-		it("rejects SELECT followed by a write statement", () => {
-			expect(() =>
-				assertReadOnlyQuery("SELECT 1; DROP TABLE foo"),
-			).toThrowError("Only a single statement is allowed");
-		});
-	});
-
-	describe("non-SELECT openers", () => {
-		it("rejects a bare INSERT", () => {
-			expect(() =>
-				assertReadOnlyQuery("INSERT INTO foo VALUES (1)"),
-			).toThrowError("Only read-only SELECT");
-		});
-
-		it("rejects UPDATE", () => {
-			expect(() => assertReadOnlyQuery("UPDATE foo SET bar = 1")).toThrowError(
-				"Only read-only SELECT",
-			);
-		});
-
-		it("rejects DELETE", () => {
-			expect(() => assertReadOnlyQuery("DELETE FROM foo")).toThrowError(
-				"Only read-only SELECT",
-			);
-		});
-
-		it("rejects DROP TABLE", () => {
-			expect(() => assertReadOnlyQuery("DROP TABLE foo")).toThrowError(
-				"Only read-only SELECT",
-			);
-		});
-
-		it("rejects CREATE TABLE", () => {
-			expect(() =>
-				assertReadOnlyQuery("CREATE TABLE foo (id INTEGER)"),
-			).toThrowError("Only read-only SELECT");
-		});
-
-		it("rejects PRAGMA", () => {
-			expect(() => assertReadOnlyQuery("PRAGMA table_info(foo)")).toThrowError(
-				"Only read-only SELECT",
-			);
-		});
-	});
-
-	describe("ATTACH / DETACH", () => {
-		it("rejects ATTACH stacked after a SELECT", () => {
-			expect(() =>
-				assertReadOnlyQuery("SELECT * FROM foo; ATTACH DATABASE 'x' AS y"),
-			).toThrowError("Only a single statement is allowed");
-		});
-
-		it("rejects ATTACH as a standalone statement opener", () => {
-			expect(() =>
-				assertReadOnlyQuery("ATTACH DATABASE 'evil.db' AS e"),
-			).toThrowError("Only read-only SELECT");
-		});
-
-		it("rejects DETACH as a standalone statement opener", () => {
-			expect(() => assertReadOnlyQuery("DETACH DATABASE e")).toThrowError(
-				"Only read-only SELECT",
-			);
-		});
-	});
-
-	describe("write enforcement delegated to the engine", () => {
-		it("passes a WITH … DELETE CTE through the static guard", () => {
-			const q = "WITH x AS (SELECT 1) DELETE FROM foo";
-			expect(assertReadOnlyQuery(q)).toBe(q);
-		});
-
-		it("passes a SELECT mentioning a write keyword through the static guard", () => {
-			const q = "SELECT * FROM foo WHERE note = 'please update'";
-			expect(assertReadOnlyQuery(q)).toBe(q);
-		});
-	});
-});
diff --git a/test/tools/query-save.test.ts b/test/tools/query-save.test.ts
index c0f9d07..818cf57 100644
--- a/test/tools/query-save.test.ts
+++ b/test/tools/query-save.test.ts
@@ -1,5 +1,8 @@
 import { beforeEach, describe, expect, it } from "vitest";
-import { registerQuerySave } from "../../src/tools/query-save";
+import {
+	assertReadOnlyQuery,
+	registerQuerySave,
+} from "../../src/tools/query-save";
 import { saveFixtures } from "../fixtures/save.fixture";
 import { createMockMcpServer } from "../mocks/mock-mcp-server";
 import type { MockMcpServer } from "../mocks/mock-mcp-server";
@@ -17,35 +20,210 @@ describe("querySave", () => {
 		expect(mcp.registerTool).toHaveBeenCalledOnce();
 	});
 
-	it.each(saveFixtures)(
-		"runs a read-only SELECT against %s",
-		async (_name, path) => {
-			const result = await mcp.callTool("pcm_query_save", {
-				savePath: path,
-				query: "SELECT COUNT(*) AS n FROM STA_race",
-			});
+	it.each(
+		saveFixtures,
+	)("runs a read-only SELECT against %s", async (_name, path) => {
+		const result = await mcp.callTool("pcm_query_save", {
+			savePath: path,
+			query: "SELECT COUNT(*) AS n FROM STA_race",
+		});
 
-			expect(result.isError).toBeUndefined();
-			expect(result.structuredContent).toMatchObject({ rowCount: 1 });
-		},
-	);
+		expect(result.isError).toBeUndefined();
+		expect(result.structuredContent).toMatchObject({ rowCount: 1 });
+	});
 
 	// A `WITH … DELETE` CTE slips past the static SELECT/WITH guard, so the write
 	// is only stopped by `PRAGMA query_only = ON` in the engine. That surfaces a
 	// "readonly database" error, which explainQueryError maps to a friendly
 	// message — this covers that branch end-to-end.
-	it.each(saveFixtures)(
-		"maps a query_only write rejection to a read-only message for %s",
-		async (_name, path) => {
-			const result = await mcp.callTool("pcm_query_save", {
-				savePath: path,
-				query: "WITH x AS (SELECT 1) DELETE FROM STA_race",
-			});
-
-			expect(result.isError).toBe(true);
-			expect(result.content[0].text).toBe(
-				"This tool is read-only — the query attempted to modify the save, which is not allowed.",
-			);
-		},
-	);
+	it.each(
+		saveFixtures,
+	)("maps a query_only write rejection to a read-only message for %s", async (_name, path) => {
+		const result = await mcp.callTool("pcm_query_save", {
+			savePath: path,
+			query: "WITH x AS (SELECT 1) DELETE FROM STA_race",
+		});
+
+		expect(result.isError).toBe(true);
+		expect(result.content).toEqual([
+			{
+				type: "text",
+				text: "This tool is read-only — the query attempted to modify the save, which is not allowed.",
+			},
+		]);
+	});
+
+	describe("assertReadOnlyQuery", () => {
+		describe("allowed queries", () => {
+			it("accepts a simple SELECT", () => {
+				const q = "SELECT * FROM DYN_cyclist";
+				expect(assertReadOnlyQuery(q)).toBe(q);
+			});
+
+			it("accepts SELECT with a trailing semicolon (strips it)", () => {
+				expect(assertReadOnlyQuery("SELECT 1;")).toBe("SELECT 1");
+			});
+
+			it("accepts SELECT with trailing semicolon and whitespace", () => {
+				// trim() runs before the semicolon strip, so a space before ';' is preserved
+				expect(assertReadOnlyQuery("SELECT 1 ;  ")).toBe("SELECT 1 ");
+			});
+
+			it("accepts SELECT with leading/trailing whitespace", () => {
+				expect(assertReadOnlyQuery("  SELECT id FROM foo  ")).toBe(
+					"SELECT id FROM foo",
+				);
+			});
+
+			it("accepts a WITH … SELECT (CTE)", () => {
+				const q =
+					"WITH cte AS (SELECT id FROM foo) SELECT * FROM cte WHERE id > 5";
+				expect(assertReadOnlyQuery(q)).toBe(q);
+			});
+
+			it("accepts lowercase select", () => {
+				expect(assertReadOnlyQuery("select * from bar")).toBe(
+					"select * from bar",
+				);
+			});
+
+			it("accepts mixed-case SELECT", () => {
+				expect(assertReadOnlyQuery("Select id From foo")).toBe(
+					"Select id From foo",
+				);
+			});
+
+			it("accepts SELECT containing a column named 'update_date' (keyword inside identifier)", () => {
+				const q = "SELECT update_date FROM DYN_cyclist";
+				expect(assertReadOnlyQuery(q)).toBe(q);
+			});
+
+			it("accepts a SELECT whose string literal contains a keyword like 'create'", () => {
+				const q = "SELECT * FROM foo WHERE name LIKE '%create%'";
+				expect(assertReadOnlyQuery(q)).toBe(q);
+			});
+
+			it("accepts a SELECT using the read-only REPLACE() function", () => {
+				const q = "SELECT REPLACE(name,'a','b') FROM foo";
+				expect(assertReadOnlyQuery(q)).toBe(q);
+			});
+
+			it("accepts SELECT with ORDER BY, LIMIT, GROUP BY", () => {
+				const q =
+					"SELECT name, COUNT(*) AS n FROM foo GROUP BY name ORDER BY n DESC LIMIT 10";
+				expect(assertReadOnlyQuery(q)).toBe(q);
+			});
+
+			it("accepts SELECT with a JOIN", () => {
+				const q = "SELECT a.id, b.name FROM a INNER JOIN b ON a.id = b.a_id";
+				expect(assertReadOnlyQuery(q)).toBe(q);
+			});
+		});
+
+		describe("empty / blank queries", () => {
+			it("rejects an empty string", () => {
+				expect(() => assertReadOnlyQuery("")).toThrowError("Query is empty.");
+			});
+
+			it("rejects a string that is only whitespace", () => {
+				expect(() => assertReadOnlyQuery("   ")).toThrowError(
+					"Query is empty.",
+				);
+			});
+
+			it("rejects a bare semicolon", () => {
+				expect(() => assertReadOnlyQuery(";")).toThrowError("Query is empty.");
+			});
+
+			it("rejects whitespace + semicolon", () => {
+				expect(() => assertReadOnlyQuery("  ;  ")).toThrowError(
+					"Query is empty.",
+				);
+			});
+		});
+
+		describe("multiple statements", () => {
+			it("rejects two SELECT statements separated by a semicolon", () => {
+				expect(() => assertReadOnlyQuery("SELECT 1; SELECT 2")).toThrowError(
+					"Only a single statement is allowed",
+				);
+			});
+
+			it("rejects SELECT followed by a write statement", () => {
+				expect(() =>
+					assertReadOnlyQuery("SELECT 1; DROP TABLE foo"),
+				).toThrowError("Only a single statement is allowed");
+			});
+		});
+
+		describe("non-SELECT openers", () => {
+			it("rejects a bare INSERT", () => {
+				expect(() =>
+					assertReadOnlyQuery("INSERT INTO foo VALUES (1)"),
+				).toThrowError("Only read-only SELECT");
+			});
+
+			it("rejects UPDATE", () => {
+				expect(() =>
+					assertReadOnlyQuery("UPDATE foo SET bar = 1"),
+				).toThrowError("Only read-only SELECT");
+			});
+
+			it("rejects DELETE", () => {
+				expect(() => assertReadOnlyQuery("DELETE FROM foo")).toThrowError(
+					"Only read-only SELECT",
+				);
+			});
+
+			it("rejects DROP TABLE", () => {
+				expect(() => assertReadOnlyQuery("DROP TABLE foo")).toThrowError(
+					"Only read-only SELECT",
+				);
+			});
+
+			it("rejects CREATE TABLE", () => {
+				expect(() =>
+					assertReadOnlyQuery("CREATE TABLE foo (id INTEGER)"),
+				).toThrowError("Only read-only SELECT");
+			});
+
+			it("rejects PRAGMA", () => {
+				expect(() =>
+					assertReadOnlyQuery("PRAGMA table_info(foo)"),
+				).toThrowError("Only read-only SELECT");
+			});
+		});
+
+		describe("ATTACH / DETACH", () => {
+			it("rejects ATTACH stacked after a SELECT", () => {
+				expect(() =>
+					assertReadOnlyQuery("SELECT * FROM foo; ATTACH DATABASE 'x' AS y"),
+				).toThrowError("Only a single statement is allowed");
+			});
+
+			it("rejects ATTACH as a standalone statement opener", () => {
+				expect(() =>
+					assertReadOnlyQuery("ATTACH DATABASE 'evil.db' AS e"),
+				).toThrowError("Only read-only SELECT");
+			});
+
+			it("rejects DETACH as a standalone statement opener", () => {
+				expect(() => assertReadOnlyQuery("DETACH DATABASE e")).toThrowError(
+					"Only read-only SELECT",
+				);
+			});
+		});
+
+		describe("write enforcement delegated to the engine", () => {
+			it("passes a WITH … DELETE CTE through the static guard", () => {
+				const q = "WITH x AS (SELECT 1) DELETE FROM foo";
+				expect(assertReadOnlyQuery(q)).toBe(q);
+			});
+
+			it("passes a SELECT mentioning a write keyword through the static guard", () => {
+				const q = "SELECT * FROM foo WHERE note = 'please update'";
+				expect(assertReadOnlyQuery(q)).toBe(q);
+			});
+		});
+	});
 });

From 19a2df28cea15c77e30c3360859018a59a3d0459 Mon Sep 17 00:00:00 2001
From: Mathieu Picciolli <mathieu.picciolli@gmail.com>
Date: Thu, 2 Jul 2026 18:06:28 -0400
Subject: [PATCH 5/7] feat: enhance assertReadOnlyQuery to handle semicolons in
 string literals, comments, and quoted identifiers

---
 src/tools/query-save.ts       | 15 ++++++++++++++-
 test/tools/query-save.test.ts | 26 ++++++++++++++++++++++++++
 2 files changed, 40 insertions(+), 1 deletion(-)

diff --git a/src/tools/query-save.ts b/src/tools/query-save.ts
index c917d0a..cc38483 100644
--- a/src/tools/query-save.ts
+++ b/src/tools/query-save.ts
@@ -119,6 +119,19 @@ function explainQueryError(error: unknown): Error {
 	return error instanceof Error ? error : new Error(message);
 }
 
+/**
+ * Matches a single SQL token whose contents should be ignored by structural
+ * scans: a string literal (`'…'`), a quoted identifier (`"…"`, backtick-quoted
+ * or `[…]`), or a line/block comment. Doubled-quote escaping (`''`, `""`) is
+ * handled by the alternations.
+ */
+const SQL_TEXT =
+	/'(?:[^']|'')*'|"(?:[^"]|"")*"|`(?:[^`]|``)*`|\[[^\]]*\]|--[^\n]*|\/\*[\s\S]*?\*\//g;
+
+function maskSqlText(sql: string): string {
+	return sql.replace(SQL_TEXT, " ");
+}
+
 /**
  * Enforce that a query is a single statement that opens as a read `SELECT`/`WITH`.
  *
@@ -141,7 +154,7 @@ export function assertReadOnlyQuery(rawQuery: string): string {
 		throw new Error("Query is empty.");
 	}
 
-	if (query.includes(";")) {
+	if (maskSqlText(query).includes(";")) {
 		throw new Error(
 			"Only a single statement is allowed — remove extra semicolons.",
 		);
diff --git a/test/tools/query-save.test.ts b/test/tools/query-save.test.ts
index 818cf57..932c924 100644
--- a/test/tools/query-save.test.ts
+++ b/test/tools/query-save.test.ts
@@ -103,6 +103,32 @@ describe("querySave", () => {
 				expect(assertReadOnlyQuery(q)).toBe(q);
 			});
 
+			it("accepts a SELECT with a semicolon inside a string literal", () => {
+				const q = "SELECT ';' AS semi";
+				expect(assertReadOnlyQuery(q)).toBe(q);
+			});
+
+			it("accepts a SELECT whose WHERE literal contains a semicolon", () => {
+				const q = "SELECT * FROM foo WHERE note = 'a;b'";
+				expect(assertReadOnlyQuery(q)).toBe(q);
+			});
+
+			it("accepts a SELECT with a semicolon inside a quoted identifier", () => {
+				const q = 'SELECT "weird;col" FROM foo';
+				expect(assertReadOnlyQuery(q)).toBe(q);
+			});
+
+			it("accepts a SELECT with a semicolon inside a comment", () => {
+				const q = "SELECT 1 -- ; not a statement";
+				expect(assertReadOnlyQuery(q)).toBe(q);
+			});
+
+			it("keeps the trailing-semicolon strip working alongside a literal semicolon", () => {
+				expect(assertReadOnlyQuery("SELECT ';' AS semi;")).toBe(
+					"SELECT ';' AS semi",
+				);
+			});
+
 			it("accepts a SELECT using the read-only REPLACE() function", () => {
 				const q = "SELECT REPLACE(name,'a','b') FROM foo";
 				expect(assertReadOnlyQuery(q)).toBe(q);

From 548fccec66aebdf31dc3a5609e1a6eed4d757220 Mon Sep 17 00:00:00 2001
From: Mathieu Picciolli <mathieu.picciolli@gmail.com>
Date: Thu, 2 Jul 2026 18:18:56 -0400
Subject: [PATCH 6/7] feat: add sql-query-identifier for improved read-only
 query enforcement

---
 package-lock.json             | 10 ++++++++
 package.json                  |  1 +
 src/tools/query-save.ts       | 46 +++++++++++++++--------------------
 test/tools/query-save.test.ts | 27 +++++++++++---------
 4 files changed, 46 insertions(+), 38 deletions(-)

diff --git a/package-lock.json b/package-lock.json
index 7f85727..d05f78a 100644
--- a/package-lock.json
+++ b/package-lock.json
@@ -12,6 +12,7 @@
 				"@anthropic-ai/mcpb": "^2.1.2",
 				"@modelcontextprotocol/sdk": "^1.29.0",
 				"cdb-converter": "^0.1.2",
+				"sql-query-identifier": "^3.1.1",
 				"sql.js": "^1.14.1",
 				"zod": "^3.25.76"
 			},
@@ -4746,6 +4747,15 @@
 				"node": ">=0.10.0"
 			}
 		},
+		"node_modules/sql-query-identifier": {
+			"version": "3.1.1",
+			"resolved": "https://registry.npmjs.org/sql-query-identifier/-/sql-query-identifier-3.1.1.tgz",
+			"integrity": "sha512-yKLx1hMFM5RKyNkFRCJrOFCPX/ZLhVMl45PWdOH1+UuEX3khELbCdgZuBnSaU9hpVvdYGMPqWc5JAAE27aybRA==",
+			"license": "MIT",
+			"engines": {
+				"node": ">= 10.13"
+			}
+		},
 		"node_modules/sql.js": {
 			"version": "1.14.1",
 			"resolved": "https://registry.npmjs.org/sql.js/-/sql.js-1.14.1.tgz",
diff --git a/package.json b/package.json
index facf5da..ecd5de0 100644
--- a/package.json
+++ b/package.json
@@ -51,6 +51,7 @@
 		"@anthropic-ai/mcpb": "^2.1.2",
 		"@modelcontextprotocol/sdk": "^1.29.0",
 		"cdb-converter": "^0.1.2",
+		"sql-query-identifier": "^3.1.1",
 		"sql.js": "^1.14.1",
 		"zod": "^3.25.76"
 	}
diff --git a/src/tools/query-save.ts b/src/tools/query-save.ts
index cc38483..5f0fa86 100644
--- a/src/tools/query-save.ts
+++ b/src/tools/query-save.ts
@@ -1,4 +1,5 @@
 import type { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js";
+import { identify } from "sql-query-identifier";
 import { z } from "zod";
 import { withSaveDb } from "../save-db";
 
@@ -120,47 +121,38 @@ function explainQueryError(error: unknown): Error {
 }
 
 /**
- * Matches a single SQL token whose contents should be ignored by structural
- * scans: a string literal (`'…'`), a quoted identifier (`"…"`, backtick-quoted
- * or `[…]`), or a line/block comment. Doubled-quote escaping (`''`, `""`) is
- * handled by the alternations.
- */
-const SQL_TEXT =
-	/'(?:[^']|'')*'|"(?:[^"]|"")*"|`(?:[^`]|``)*`|\[[^\]]*\]|--[^\n]*|\/\*[\s\S]*?\*\//g;
-
-function maskSqlText(sql: string): string {
-	return sql.replace(SQL_TEXT, " ");
-}
-
-/**
- * Enforce that a query is a single statement that opens as a read `SELECT`/`WITH`.
+ * Enforce that a query is a single read-only statement.
  *
- * Actual write protection is delegated to the SQLite engine via
- * `PRAGMA query_only = ON` (see {@link withSaveDb}), which reliably rejects any
- * mutating statement — including tricks a text scan would miss, such as a
- * `WITH … DELETE` CTE. These static checks only cover what the engine can't:
- *  - blocking stacked statements (SQLite prepares just the first one anyway, so
- *    the extra semicolon check keeps intent explicit and errors clear), which
- *    also shuts out `ATTACH`/`DETACH` since those can only appear as their own
- *    statement, and
- *  - giving a fast, friendly error for an obviously non-read opener.
+ * Parsing is delegated to `sql-query-identifier`, which tokenizes SQL properly:
+ * a `;` inside a string literal, comment or quoted identifier is not mistaken
+ * for a statement separator, and CTEs are classified by their leaf operation —
+ * `WITH … SELECT` reads (`LISTING`) while `WITH … DELETE` writes
+ * (`MODIFICATION`). Anything that isn't exactly one `LISTING` statement is
+ * rejected here; `PRAGMA query_only = ON` (see {@link withSaveDb}) stays as the
+ * engine-level backstop.
  */
 export function assertReadOnlyQuery(rawQuery: string): string {
-	// Strip a single trailing semicolon, then reject any further statement
-	// separators to prevent stacked statements.
+	// Strip a single trailing semicolon so a normal `SELECT …;` is accepted;
+	// the returned query is what gets prepared.
 	const query = rawQuery.trim().replace(/;\s*$/, "");
 
 	if (query.length === 0) {
 		throw new Error("Query is empty.");
 	}
 
-	if (maskSqlText(query).includes(";")) {
+	const statements = identify(query, { strict: false, dialect: "sqlite" });
+
+	if (statements.length === 0) {
+		throw new Error("Query is empty.");
+	}
+
+	if (statements.length > 1) {
 		throw new Error(
 			"Only a single statement is allowed — remove extra semicolons.",
 		);
 	}
 
-	if (!/^(select|with)\b/i.test(query)) {
+	if (statements[0].executionType !== "LISTING") {
 		throw new Error(
 			"Only read-only SELECT (or WITH … SELECT) queries are allowed.",
 		);
diff --git a/test/tools/query-save.test.ts b/test/tools/query-save.test.ts
index 932c924..6784f2b 100644
--- a/test/tools/query-save.test.ts
+++ b/test/tools/query-save.test.ts
@@ -32,13 +32,9 @@ describe("querySave", () => {
 		expect(result.structuredContent).toMatchObject({ rowCount: 1 });
 	});
 
-	// A `WITH … DELETE` CTE slips past the static SELECT/WITH guard, so the write
-	// is only stopped by `PRAGMA query_only = ON` in the engine. That surfaces a
-	// "readonly database" error, which explainQueryError maps to a friendly
-	// message — this covers that branch end-to-end.
 	it.each(
 		saveFixtures,
-	)("maps a query_only write rejection to a read-only message for %s", async (_name, path) => {
+	)("rejects a WITH … DELETE CTE before touching the save for %s", async (_name, path) => {
 		const result = await mcp.callTool("pcm_query_save", {
 			savePath: path,
 			query: "WITH x AS (SELECT 1) DELETE FROM STA_race",
@@ -48,7 +44,7 @@ describe("querySave", () => {
 		expect(result.content).toEqual([
 			{
 				type: "text",
-				text: "This tool is read-only — the query attempted to modify the save, which is not allowed.",
+				text: "Only read-only SELECT (or WITH … SELECT) queries are allowed.",
 			},
 		]);
 	});
@@ -240,13 +236,22 @@ describe("querySave", () => {
 			});
 		});
 
-		describe("write enforcement delegated to the engine", () => {
-			it("passes a WITH … DELETE CTE through the static guard", () => {
-				const q = "WITH x AS (SELECT 1) DELETE FROM foo";
-				expect(assertReadOnlyQuery(q)).toBe(q);
+		describe("write detection via the statement identifier", () => {
+			it("rejects a WITH … DELETE CTE (write behind a read opener)", () => {
+				expect(() =>
+					assertReadOnlyQuery("WITH x AS (SELECT 1) DELETE FROM foo"),
+				).toThrowError("Only read-only SELECT");
+			});
+
+			it("rejects a WITH … INSERT CTE", () => {
+				expect(() =>
+					assertReadOnlyQuery(
+						"WITH x AS (SELECT 1) INSERT INTO foo SELECT * FROM x",
+					),
+				).toThrowError("Only read-only SELECT");
 			});
 
-			it("passes a SELECT mentioning a write keyword through the static guard", () => {
+			it("accepts a SELECT merely mentioning a write keyword in a literal", () => {
 				const q = "SELECT * FROM foo WHERE note = 'please update'";
 				expect(assertReadOnlyQuery(q)).toBe(q);
 			});

From b80b9242518106ebd7b2bb2b2f47f0ce010b7b78 Mon Sep 17 00:00:00 2001
From: Mathieu Picciolli <mathieu.picciolli@gmail.com>
Date: Thu, 2 Jul 2026 18:23:56 -0400
Subject: [PATCH 7/7] Potential fix for pull request finding

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
---
 test/tools/query-save.test.ts | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/test/tools/query-save.test.ts b/test/tools/query-save.test.ts
index 6784f2b..4605c63 100644
--- a/test/tools/query-save.test.ts
+++ b/test/tools/query-save.test.ts
@@ -34,7 +34,7 @@ describe("querySave", () => {
 
 	it.each(
 		saveFixtures,
-	)("rejects a WITH … DELETE CTE before touching the save for %s", async (_name, path) => {
+	)("rejects a WITH … DELETE CTE for %s", async (_name, path) => {
 		const result = await mcp.callTool("pcm_query_save", {
 			savePath: path,
 			query: "WITH x AS (SELECT 1) DELETE FROM STA_race",