From 62eb43e1eb282931a5bd9dfa613f365b6009fbaf Mon Sep 17 00:00:00 2001 From: Mathieu Picciolli Date: Thu, 2 Jul 2026 17:39:00 -0400 Subject: [PATCH 1/8] feat: implement pcm_update_save tool for writing changes to new .cdb files --- README.md | 12 +- src/helpers.ts | 33 +++++ src/save-db.ts | 52 +++++++- src/tools/index.ts | 2 + src/tools/query-save.ts | 34 +---- src/tools/update-save.ts | 112 +++++++++++++++++ test/tools/update-save.test.ts | 222 +++++++++++++++++++++++++++++++++ 7 files changed, 426 insertions(+), 41 deletions(-) create mode 100644 src/tools/update-save.ts create mode 100644 test/tools/update-save.test.ts diff --git a/README.md b/README.md index cddd1ba..c202b4b 100644 --- a/README.md +++ b/README.md @@ -18,7 +18,7 @@ `pcm-mcp` is a [Model Context Protocol](https://modelcontextprotocol.io) server that lets AI assistants such as Claude Desktop, ChatGPT and Gemini query your [Pro Cycling Manager](https://www.cyanide-studio.com/) (PCM) game saves. Ask about a rider's ratings, browse a team's roster, run SQL against the save, or generate a race startlist — all in plain language. > [!IMPORTANT] -> This server is strictly **read-only**. PCM stores careers as binary `.cdb` files; each call re-reads the `.cdb` from disk and loads it into an **in-memory** SQLite database. Your save files are **never written to or modified**. +> This server never modifies your existing save files. PCM stores careers as binary `.cdb` files; each call re-reads the `.cdb` from disk and loads it into an **in-memory** SQLite database. Every read tool leaves the source untouched. The single write tool, `pcm_update_save`, serializes its changes to a **new** `.cdb` file (`outputPath`) and refuses to overwrite the input — keep your original save as a backup. ## Features @@ -26,8 +26,9 @@ - **Save discovery** — auto-detect PCM career saves on Windows, or point at any `.cdb` file directly. - **Rich queries** — search cyclists and teams, inspect rosters with full per-terrain ratings, and read player info. - **Raw SQL** — run guarded, read-only `SELECT` queries against any table in the save. +- **Guarded edits** — apply a single `INSERT`/`UPDATE`/`DELETE` and write the result to a new `.cdb`, never touching the original. - **Startlist export** — generate a PCM-ready startlist XML from a set of teams and rosters. -- **Safe by design** — every tool is annotated `readOnlyHint: true`, so clients can auto-approve them without prompts. +- **Safe by design** — read tools are annotated `readOnlyHint: true` for auto-approval; the write tool writes only to a separate output file. ## Getting started @@ -80,7 +81,7 @@ Auto-discovery via `pcm_list_saves` is therefore **Windows only**. On macOS/Linu ## Available tools -All tools are prefixed with `pcm_`, are read-only, and carry `readOnlyHint: true` so clients like Claude Desktop can approve them automatically without a confirmation prompt. +All tools are prefixed with `pcm_`. Every tool except `pcm_update_save` is read-only and carries `readOnlyHint: true` so clients like Claude Desktop can approve them automatically without a confirmation prompt. `pcm_update_save` is the one write tool; it never overwrites the source save. | Tool | Description | | ------------------------------ | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | @@ -93,15 +94,16 @@ All tools are prefixed with `pcm_`, are read-only, and carry `readOnlyHint: true | **pcm_get_team_roster** | List a team's roster (defaults to the active player's team when `teamId` is omitted). Joins DYN_cyclist with its active DYN_contract_cyclist and STA_type_rider; per rider returns name, country, age (derived from birth date and the current game date), rider type, overall ability, contract end year, wage, market value and all per-terrain ability ratings. Ordered by overall ability, highest first. Errors if `teamId` does not exist. | | **pcm_search_team** | Search for a team by name (case-insensitive partial match against both the full name and short name). Returns up to 10 matches with the resolved division name, country name, evaluation and general manager. | | **pcm_query_save** | Run a read-only SQL query (`SELECT` / `WITH … SELECT` only) against any table in a save file. Write/DDL statements are rejected. Results are capped (default 100, max 1000 rows). | +| **pcm_update_save** | Apply a single `INSERT`/`UPDATE`/`DELETE` statement to a save and write the modified database to a **new** `.cdb` at `outputPath`. The source save is never overwritten (`outputPath` must differ from `savePath`); `SELECT`, schema changes (`DROP`/`CREATE`/`ALTER`) and stacked statements are rejected. Returns the written path and the number of rows changed. | | **pcm_generate_startlist_xml** | Generate a PCM startlist XML document from a list of teams and their cyclist rosters. Looks up the race by `IDrace` in the save to derive the output file name from `STA_race.gene_sz_filename` (e.g. `c0_almeria.xml`), and returns both the file name and the XML as text. Team and cyclist IDs map to `DYN_team.IDteam` / `DYN_cyclist.IDcyclist` (look them up with `pcm_search_cyclist` or `pcm_query_save`). | ## How it works -Tools are **stateless**: there is no "current save" held by the server. Every save-reading tool takes an absolute `savePath`, re-validates it, and re-reads the `.cdb` from disk into a fresh in-memory SQLite database (via [`cdb-converter`](https://www.npmjs.com/package/cdb-converter) + [`sql.js`](https://www.npmjs.com/package/sql.js)) for each call. The on-disk save is the single source of truth and is never mutated. A typical flow is: +Tools are **stateless**: there is no "current save" held by the server. Every tool takes an absolute `savePath`, re-validates it, and re-reads the `.cdb` from disk into a fresh in-memory SQLite database (via [`cdb-converter`](https://www.npmjs.com/package/cdb-converter) + [`sql.js`](https://www.npmjs.com/package/sql.js)) for each call. The source save on disk is never mutated: read tools only ever read it, and `pcm_update_save` writes its changes to a separate output `.cdb`. A typical flow is: 1. `pcm_list_saves` (Windows) or `pcm_select_save` with an explicit path to locate a save. 2. `pcm_search_cyclist`, `pcm_get_team_roster`, `pcm_query_save`, … to explore it. -3. `pcm_generate_startlist_xml` to produce a startlist file for a race. +3. `pcm_generate_startlist_xml` to produce a startlist file for a race, or `pcm_update_save` to write an edited copy of the save. ## Development diff --git a/src/helpers.ts b/src/helpers.ts index 8f9ec33..162d65d 100644 --- a/src/helpers.ts +++ b/src/helpers.ts @@ -60,6 +60,39 @@ export function buildStartlistXml(teams: StartlistTeam[]): string { return `${lines.join("\n")}\n`; } +/** + * Translate sql.js "no such table/column" errors into actionable messages that + * point the caller at the schema-discovery tools. Other errors pass through. + * + * Shared by the read (`pcm_query_save`) and write (`pcm_update_save`) tools. + */ +export function explainQueryError(error: unknown): Error { + const message = error instanceof Error ? error.message : String(error); + + const missingTable = /no such table:\s*(\S+)/i.exec(message); + if (missingTable) { + return new Error( + `Table "${missingTable[1]}" does not exist in this save — use pcm_get_save_schema to list available tables.`, + ); + } + + const missingColumn = /no such column:\s*(\S+)/i.exec(message); + if (missingColumn) { + return new Error( + `Column "${missingColumn[1]}" does not exist — use pcm_get_table_schema to inspect the table's columns.`, + ); + } + + // 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); +} + /** Compute age in whole years from two YYYYMMDD integers (e.g. 20030503). */ export function ageFromYmd(currentYmd: number, birthYmd: number): number { let age = Math.floor(currentYmd / 10000) - Math.floor(birthYmd / 10000); diff --git a/src/save-db.ts b/src/save-db.ts index cbdae83..3d89850 100644 --- a/src/save-db.ts +++ b/src/save-db.ts @@ -1,6 +1,7 @@ -import { readFile } from "node:fs/promises"; +import { readFile, writeFile } from "node:fs/promises"; +import { resolve } from "node:path"; import type { CallToolResult } from "@modelcontextprotocol/sdk/types.js"; -import { cdbToSql } from "cdb-converter"; +import { cdbToSql, sqlToCdb } from "cdb-converter"; import initSqlJs from "sql.js"; import { errorResponse, validResponse } from "./helpers"; import { type SaveFile, validateSave } from "./saves"; @@ -48,7 +49,10 @@ export function getGameDate(db: SaveDb): number | null { * - turns thrown errors into an {@link errorResponse} and the returned value * into a {@link validResponse}. * - * The save is loaded into memory only; changes are never written back to disk. + * `withSaveDb` itself never writes to `savePath`: the on-disk source save is + * only ever read. A write-capable tool can pass `{ queryOnly: false }`, mutate + * the in-memory database in `fn`, and serialize the result to a *separate* + * output file via {@link writeSaveDb} — the source is never overwritten. * * @param savePath - Absolute path to the `.cdb` save file. * @param fn - Receives the open database and the validated save metadata, and @@ -57,6 +61,11 @@ export function getGameDate(db: SaveDb): number | null { export async function withSaveDb>( savePath: string, fn: (db: SaveDb, save: SaveFile) => T | Promise, + config: { + queryOnly?: boolean; + } = { + queryOnly: true, + }, ): Promise { let db: SaveDb | undefined; try { @@ -66,7 +75,9 @@ export async function withSaveDb>( const cdbBuffer = await readFile(save.path); db = cdbToSql(cdbBuffer, SQL); - db.run("PRAGMA query_only = ON;"); + if (config.queryOnly) { + db.run("PRAGMA query_only = ON;"); + } const output = await fn(db, save); @@ -79,3 +90,36 @@ export async function withSaveDb>( db?.close(); } } + +/** + * Serialize an edited in-memory save back to a `.cdb` file at `outputPath`. + * + * Writes only ever go to a new file: this refuses to overwrite the source save + * (`sourcePath`), so the input `.cdb` is never modified. `sqlToCdb` re-encodes + * the sql.js database into PCM's compressed `.cdb` binary format. + * + * @param db - The (edited) in-memory database to serialize. + * @param outputPath - Absolute path of the `.cdb` file to write. + * @param sourcePath - Absolute path of the source save, used only to guard + * against overwriting it. + * @returns The absolute path written. + * @throws if `outputPath` isn't a `.cdb` file or resolves to `sourcePath`. + */ +export async function writeSaveDb( + db: SaveDb, + outputPath: string, + sourcePath: string, +): Promise { + if (!outputPath.toLowerCase().endsWith(".cdb")) { + throw new Error(`Output must be a .cdb file: ${outputPath}`); + } + if (resolve(outputPath) === resolve(sourcePath)) { + throw new Error( + "outputPath must differ from the source save — the input .cdb is never overwritten.", + ); + } + + const cdb = sqlToCdb(db); + await writeFile(outputPath, Buffer.from(cdb)); + return resolve(outputPath); +} diff --git a/src/tools/index.ts b/src/tools/index.ts index 5221cb5..ae3c7ef 100644 --- a/src/tools/index.ts +++ b/src/tools/index.ts @@ -6,6 +6,7 @@ import { registerGetTableSchema } from "./get-table-schema"; import { registerGetPlayerInfo } from "./get-player-info"; import { registerGetTeamRoster } from "./get-team-roster"; import { registerQuerySave } from "./query-save"; +import { registerUpdateSave } from "./update-save"; import { registerSearchCyclist } from "./search-cyclist"; import { registerGenerateStartlistXml } from "./generate-startlist-xml"; import { registerSearchTeam } from "./search-team"; @@ -18,6 +19,7 @@ export function registerTools(server: McpServer): void { registerGetPlayerInfo(server); registerGetTeamRoster(server); registerQuerySave(server); + registerUpdateSave(server); registerSearchCyclist(server); registerGenerateStartlistXml(server); registerSearchTeam(server); diff --git a/src/tools/query-save.ts b/src/tools/query-save.ts index 5f0fa86..b91d4fd 100644 --- a/src/tools/query-save.ts +++ b/src/tools/query-save.ts @@ -1,6 +1,7 @@ import type { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js"; import { identify } from "sql-query-identifier"; import { z } from "zod"; +import { explainQueryError } from "../helpers"; import { withSaveDb } from "../save-db"; const DEFAULT_LIMIT = 100; @@ -90,38 +91,7 @@ export function registerQuerySave(server: McpServer): void { } /** - * Translate sql.js "no such table/column" errors into actionable messages that - * point the caller at the schema-discovery tools. Other errors pass through. - */ -function explainQueryError(error: unknown): Error { - const message = error instanceof Error ? error.message : String(error); - - const missingTable = /no such table:\s*(\S+)/i.exec(message); - if (missingTable) { - return new Error( - `Table "${missingTable[1]}" does not exist in this save — use pcm_get_save_schema to list available tables.`, - ); - } - - const missingColumn = /no such column:\s*(\S+)/i.exec(message); - if (missingColumn) { - return new Error( - `Column "${missingColumn[1]}" does not exist — use pcm_get_table_schema to inspect the table's columns.`, - ); - } - - // 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); -} - -/** - * Enforce that a query is a single read-only statement. + * Enforce that a query is a single statement that opens as a read `SELECT`/`WITH`. * * Parsing is delegated to `sql-query-identifier`, which tokenizes SQL properly: * a `;` inside a string literal, comment or quoted identifier is not mistaken diff --git a/src/tools/update-save.ts b/src/tools/update-save.ts new file mode 100644 index 0000000..8723122 --- /dev/null +++ b/src/tools/update-save.ts @@ -0,0 +1,112 @@ +import type { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js"; +import { z } from "zod"; +import { explainQueryError } from "../helpers"; +import { withSaveDb, writeSaveDb } from "../save-db"; + +const outputSchema = z.object({ + outputPath: z + .string() + .describe("Absolute path of the modified .cdb save that was written"), + rowsModified: z + .number() + .describe("Number of rows changed by the statement (INSERT/UPDATE/DELETE)"), + statement: z + .string() + .describe("The normalized statement that was executed"), +}); + +export function registerUpdateSave(server: McpServer): void { + server.registerTool( + "pcm_update_save", + { + title: "Update PCM save (writes a new .cdb)", + description: + "Run a single write statement (INSERT, UPDATE or DELETE) against a Pro Cycling Manager `.cdb` save and write the result to a NEW `.cdb` file. The source save is never modified: the edited database is serialized to `outputPath`, which must differ from `savePath`. Only one data-mutating statement is allowed; SELECT, schema changes (DROP/CREATE/ALTER) and stacked statements are rejected. Use `pcm_query_save` to read, and `pcm_get_save_schema`/`pcm_get_table_schema` to discover tables and columns.", + inputSchema: { + savePath: z + .string() + .describe("Absolute path to the source .cdb save file"), + outputPath: z + .string() + .describe( + "Absolute path of the .cdb file to write the modified save to (must differ from savePath)", + ), + statement: z + .string() + .describe( + "A single write statement, e.g. `UPDATE DYN_cyclist SET gene_sprint = 80 WHERE IDcyclist = 42`", + ), + }, + outputSchema, + annotations: { + readOnlyHint: false, + destructiveHint: true, + idempotentHint: false, + openWorldHint: false, + }, + }, + async ({ savePath, outputPath, statement }) => + withSaveDb( + savePath, + async (db, save) => { + const safe = assertWriteStatement(statement); + + try { + db.run(safe); + } catch (error) { + throw explainQueryError(error); + } + + const rowsModified = db.getRowsModified(); + const written = await writeSaveDb(db, outputPath, save.path); + + const output: z.infer = { + outputPath: written, + rowsModified, + statement: safe, + }; + return output; + }, + { queryOnly: false }, + ), + ); +} + +/** + * Enforce that a statement is a single data-mutating write. + * + * Uses an opener whitelist (`INSERT`/`UPDATE`/`DELETE`) rather than a keyword + * blocklist: + * - it rejects reads (`SELECT`/`WITH`) — those belong to `pcm_query_save`, and + * - it rejects DDL (`DROP`/`CREATE`/`ALTER`/`PRAGMA`/…), which would alter the + * schema and break the `sqlToCdb` round-trip (it needs the table structure / + * `DB_STRUCTURE` intact to re-encode the `.cdb`). + * + * Stacked statements are rejected too (only the first would run anyway, and + * banning the extra `;` also shuts out `ATTACH`/`DETACH`, which must stand + * alone). + */ +export function assertWriteStatement(rawStatement: string): string { + // Strip a single trailing semicolon, then reject any further statement + // separators to prevent stacked statements. + const statement = rawStatement.trim().replace(/;\s*$/, ""); + + if (statement.length === 0) { + throw new Error("Statement is empty."); + } + + if (statement.includes(";")) { + throw new Error( + "Only a single statement is allowed — remove extra semicolons.", + ); + } + + if (!/^(insert|update|delete)\b/i.test(statement)) { + throw new Error( + "Only a single INSERT, UPDATE or DELETE statement is allowed. " + + "Use pcm_query_save to read; schema changes (DROP/CREATE/ALTER) are not supported.", + ); + } + + return statement; +} diff --git a/test/tools/update-save.test.ts b/test/tools/update-save.test.ts new file mode 100644 index 0000000..511b70e --- /dev/null +++ b/test/tools/update-save.test.ts @@ -0,0 +1,222 @@ +import { mkdtemp, readFile, rm, stat } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { cdbToSql } from "cdb-converter"; +import initSqlJs from "sql.js"; +import { afterEach, beforeEach, describe, expect, it } from "vitest"; +import { + assertWriteStatement, + registerUpdateSave, +} from "../../src/tools/update-save"; +import { saveFixtures } from "../fixtures/save.fixture"; +import { createMockMcpServer } from "../mocks/mock-mcp-server"; +import type { MockMcpServer } from "../mocks/mock-mcp-server"; + +/** Read `GAM_config.gene_i_date` back out of a written `.cdb` file. */ +async function readGameDate(cdbPath: string): Promise { + const SQL = await initSqlJs(); + const db = cdbToSql(await readFile(cdbPath), SQL); + try { + const result = db.exec("SELECT gene_i_date FROM GAM_config LIMIT 1"); + return Number(result[0]?.values?.[0]?.[0]); + } finally { + db.close(); + } +} + +describe("updateSave", () => { + let mcp: MockMcpServer; + let outDir: string; + + beforeEach(async () => { + mcp = createMockMcpServer(); + registerUpdateSave(mcp.server); + outDir = await mkdtemp(join(tmpdir(), "pcm-update-")); + }); + + afterEach(async () => { + await rm(outDir, { recursive: true, force: true }); + }); + + it("registers the pcm_update_save tool", () => { + expect(mcp.getTool("pcm_update_save")).toBeDefined(); + expect(mcp.registerTool).toHaveBeenCalledOnce(); + }); + + it.each( + saveFixtures, + )("applies an UPDATE and writes the change to a new .cdb for %s", async (_name, path) => { + const outputPath = join(outDir, "edited.cdb"); + const result = await mcp.callTool("pcm_update_save", { + savePath: path, + outputPath, + statement: "UPDATE GAM_config SET gene_i_date = 20991231", + }); + + expect(result.isError).toBeUndefined(); + expect(result.structuredContent).toMatchObject({ + outputPath, + rowsModified: 1, + statement: "UPDATE GAM_config SET gene_i_date = 20991231", + }); + + // The change must actually persist in the written file. + expect(await readGameDate(outputPath)).toBe(20991231); + }); + + it.each( + saveFixtures, + )("leaves the source save untouched for %s", async (_name, path) => { + const before = await stat(path); + const outputPath = join(outDir, "edited.cdb"); + + await mcp.callTool("pcm_update_save", { + savePath: path, + outputPath, + statement: "UPDATE GAM_config SET gene_i_date = 20991231", + }); + + const after = await stat(path); + expect(after.size).toBe(before.size); + expect(after.mtimeMs).toBe(before.mtimeMs); + }); + + it.each( + saveFixtures, + )("refuses to overwrite the source save for %s", async (_name, path) => { + const result = await mcp.callTool("pcm_update_save", { + savePath: path, + outputPath: path, + statement: "UPDATE GAM_config SET gene_i_date = 20991231", + }); + + expect(result.isError).toBe(true); + expect(result.content[0]).toEqual({ + type: "text", + text: "outputPath must differ from the source save — the input .cdb is never overwritten.", + }); + }); + + it.each( + saveFixtures, + )("rejects a non-.cdb output path for %s", async (_name, path) => { + const result = await mcp.callTool("pcm_update_save", { + savePath: path, + outputPath: join(outDir, "edited.txt"), + statement: "UPDATE GAM_config SET gene_i_date = 20991231", + }); + + expect(result.isError).toBe(true); + expect(result.content[0]).toEqual({ + type: "text", + text: expect.stringMatching(/must be a \.cdb file/), + }); + }); + + it.each( + saveFixtures, + )("maps a missing table to a schema-discovery hint for %s", async (_name, path) => { + const result = await mcp.callTool("pcm_update_save", { + savePath: path, + outputPath: join(outDir, "edited.cdb"), + statement: "UPDATE not_a_table SET x = 1", + }); + + expect(result.isError).toBe(true); + expect(result.content[0]).toEqual({ + type: "text", + text: 'Table \"not_a_table\" does not exist in this save — use pcm_get_save_schema to list available tables.', + }); + }); + + describe("assertWriteStatement", () => { + describe("allowed statements", () => { + it("accepts an UPDATE", () => { + const s = "UPDATE foo SET bar = 1"; + expect(assertWriteStatement(s)).toBe(s); + }); + + it("accepts an INSERT", () => { + const s = "INSERT INTO foo (id) VALUES (1)"; + expect(assertWriteStatement(s)).toBe(s); + }); + + it("accepts a DELETE", () => { + const s = "DELETE FROM foo WHERE id = 1"; + expect(assertWriteStatement(s)).toBe(s); + }); + + it("strips a single trailing semicolon", () => { + expect(assertWriteStatement("DELETE FROM foo;")).toBe( + "DELETE FROM foo", + ); + }); + + it("trims surrounding whitespace", () => { + expect(assertWriteStatement(" UPDATE foo SET x = 1 ")).toBe( + "UPDATE foo SET x = 1", + ); + }); + + it("accepts a lowercase opener", () => { + expect(assertWriteStatement("update foo set x = 1")).toBe( + "update foo set x = 1", + ); + }); + }); + + describe("empty / blank statements", () => { + it("rejects an empty string", () => { + expect(() => assertWriteStatement("")).toThrowError( + "Statement is empty.", + ); + }); + + it("rejects a bare semicolon", () => { + expect(() => assertWriteStatement(";")).toThrowError( + "Statement is empty.", + ); + }); + }); + + describe("multiple statements", () => { + it("rejects two statements separated by a semicolon", () => { + expect(() => + assertWriteStatement("UPDATE foo SET x = 1; DELETE FROM foo"), + ).toThrowError("Only a single statement is allowed"); + }); + }); + + describe("disallowed openers", () => { + it("rejects a SELECT", () => { + expect(() => assertWriteStatement("SELECT * FROM foo")).toThrowError( + "Only a single INSERT, UPDATE or DELETE", + ); + }); + + it("rejects a WITH … statement", () => { + expect(() => + assertWriteStatement("WITH x AS (SELECT 1) UPDATE foo SET a = 1"), + ).toThrowError("Only a single INSERT, UPDATE or DELETE"); + }); + + it("rejects DROP TABLE", () => { + expect(() => assertWriteStatement("DROP TABLE foo")).toThrowError( + "Only a single INSERT, UPDATE or DELETE", + ); + }); + + it("rejects CREATE TABLE", () => { + expect(() => + assertWriteStatement("CREATE TABLE foo (id INTEGER)"), + ).toThrowError("Only a single INSERT, UPDATE or DELETE"); + }); + + it("rejects ATTACH as a standalone opener", () => { + expect(() => + assertWriteStatement("ATTACH DATABASE 'evil.db' AS e"), + ).toThrowError("Only a single INSERT, UPDATE or DELETE"); + }); + }); + }); +}); From 1ef15fa3a1d406719c34ccc890f7ddad0bed177d Mon Sep 17 00:00:00 2001 From: Mathieu Picciolli Date: Thu, 2 Jul 2026 18:36:43 -0400 Subject: [PATCH 2/8] feat: add parseSingleStatement function for SQL statement normalization and validation --- src/helpers.ts | 39 +++++++++++++++++++++++++++++++ src/tools/query-save.ts | 42 +++++++++------------------------- src/tools/update-save.ts | 40 ++++++++++++-------------------- test/tools/update-save.test.ts | 16 ++++++++++--- 4 files changed, 78 insertions(+), 59 deletions(-) diff --git a/src/helpers.ts b/src/helpers.ts index 162d65d..289ca9d 100644 --- a/src/helpers.ts +++ b/src/helpers.ts @@ -1,4 +1,5 @@ import type { CallToolResult } from "@modelcontextprotocol/sdk/types.js"; +import { type Result as IdentifyResult, identify } from "sql-query-identifier"; export function validResponse( structured: @@ -93,6 +94,44 @@ export function explainQueryError(error: unknown): Error { return error instanceof Error ? error : new Error(message); } +/** + * Normalize `raw` to a single SQL statement and parse it with + * `sql-query-identifier`. + * + * Strips one trailing `;`, then rejects empty input and stacked statements + * (`label` — e.g. "Query" or "Statement" — is used in the empty-input message). + * Because the parser tokenizes SQL properly, a `;` inside a string literal, + * comment or quoted identifier is not mistaken for a statement separator. + * + * Returns the normalized text (safe to prepare/run) and the parsed statement; + * callers decide which statement kinds they allow (via `type`/`executionType`). + * + */ +export function parseSingleStatement( + raw: string, + label: string, +): { text: string; statement: IdentifyResult } { + const text = raw.trim().replace(/;\s*$/, ""); + + if (text.length === 0) { + throw new Error(`${label} is empty.`); + } + + const statements = identify(text, { strict: false, dialect: "sqlite" }); + + if (statements.length === 0) { + throw new Error(`${label} is empty.`); + } + + if (statements.length > 1) { + throw new Error( + "Only a single statement is allowed — remove extra semicolons.", + ); + } + + return { text, statement: statements[0] }; +} + /** Compute age in whole years from two YYYYMMDD integers (e.g. 20030503). */ export function ageFromYmd(currentYmd: number, birthYmd: number): number { let age = Math.floor(currentYmd / 10000) - Math.floor(birthYmd / 10000); diff --git a/src/tools/query-save.ts b/src/tools/query-save.ts index b91d4fd..dd9c127 100644 --- a/src/tools/query-save.ts +++ b/src/tools/query-save.ts @@ -1,7 +1,6 @@ import type { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js"; -import { identify } from "sql-query-identifier"; import { z } from "zod"; -import { explainQueryError } from "../helpers"; +import { explainQueryError, parseSingleStatement } from "../helpers"; import { withSaveDb } from "../save-db"; const DEFAULT_LIMIT = 100; @@ -91,42 +90,23 @@ export function registerQuerySave(server: McpServer): void { } /** - * 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. * - * 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. + * Parsing is delegated to {@link parseSingleStatement}, so a `;` inside a string + * literal, comment or quoted identifier is not mistaken for a statement + * separator. CTEs are classified by their leaf operation, so `WITH … SELECT` + * reads (`LISTING`) while `WITH … DELETE` writes (`MODIFICATION`) — only the + * former is accepted. `PRAGMA query_only = ON` (see {@link withSaveDb}) stays as + * the engine-level backstop. */ export function assertReadOnlyQuery(rawQuery: string): string { - // Strip a single trailing semicolon so a normal `SELECT …;` is accepted; - // the returned query is what gets prepared. - const query = rawQuery.trim().replace(/;\s*$/, ""); + const { text, statement } = parseSingleStatement(rawQuery, "Query"); - if (query.length === 0) { - throw new Error("Query is empty."); - } - - 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 (statements[0].executionType !== "LISTING") { + if (statement.executionType !== "LISTING") { throw new Error( "Only read-only SELECT (or WITH … SELECT) queries are allowed.", ); } - return query; + return text; } diff --git a/src/tools/update-save.ts b/src/tools/update-save.ts index 8723122..a53177b 100644 --- a/src/tools/update-save.ts +++ b/src/tools/update-save.ts @@ -1,6 +1,6 @@ import type { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js"; import { z } from "zod"; -import { explainQueryError } from "../helpers"; +import { explainQueryError, parseSingleStatement } from "../helpers"; import { withSaveDb, writeSaveDb } from "../save-db"; const outputSchema = z.object({ @@ -72,41 +72,31 @@ export function registerUpdateSave(server: McpServer): void { ); } +/** The only statement kinds this tool executes: plain data mutations. */ +const WRITE_STATEMENT_TYPES = new Set(["INSERT", "UPDATE", "DELETE"]); + /** * Enforce that a statement is a single data-mutating write. * - * Uses an opener whitelist (`INSERT`/`UPDATE`/`DELETE`) rather than a keyword - * blocklist: - * - it rejects reads (`SELECT`/`WITH`) — those belong to `pcm_query_save`, and - * - it rejects DDL (`DROP`/`CREATE`/`ALTER`/`PRAGMA`/…), which would alter the - * schema and break the `sqlToCdb` round-trip (it needs the table structure / - * `DB_STRUCTURE` intact to re-encode the `.cdb`). + * Parsing is delegated to {@link parseSingleStatement}, which classifies the + * statement by its leaf operation. Only `INSERT`/`UPDATE`/`DELETE` are allowed + * (a `WITH … DELETE` CTE counts as a `DELETE`). Everything else is rejected: + * - reads (`SELECT`, `WITH … SELECT`) — those belong to `pcm_query_save`, and + * - DDL (`DROP`/`CREATE`/`ALTER`/…) and anything unknown (`PRAGMA`, `ATTACH`), + * which would alter the schema and break the `sqlToCdb` round-trip (it needs + * the table structure / `DB_STRUCTURE` intact to re-encode the `.cdb`). * - * Stacked statements are rejected too (only the first would run anyway, and - * banning the extra `;` also shuts out `ATTACH`/`DETACH`, which must stand - * alone). + * A `;` inside a string literal no longer trips the single-statement check. */ export function assertWriteStatement(rawStatement: string): string { - // Strip a single trailing semicolon, then reject any further statement - // separators to prevent stacked statements. - const statement = rawStatement.trim().replace(/;\s*$/, ""); - - if (statement.length === 0) { - throw new Error("Statement is empty."); - } - - if (statement.includes(";")) { - throw new Error( - "Only a single statement is allowed — remove extra semicolons.", - ); - } + const { text, statement } = parseSingleStatement(rawStatement, "Statement"); - if (!/^(insert|update|delete)\b/i.test(statement)) { + if (!WRITE_STATEMENT_TYPES.has(statement.type)) { throw new Error( "Only a single INSERT, UPDATE or DELETE statement is allowed. " + "Use pcm_query_save to read; schema changes (DROP/CREATE/ALTER) are not supported.", ); } - return statement; + return text; } diff --git a/test/tools/update-save.test.ts b/test/tools/update-save.test.ts index 511b70e..5fa8c91 100644 --- a/test/tools/update-save.test.ts +++ b/test/tools/update-save.test.ts @@ -125,7 +125,7 @@ describe("updateSave", () => { expect(result.isError).toBe(true); expect(result.content[0]).toEqual({ type: "text", - text: 'Table \"not_a_table\" does not exist in this save — use pcm_get_save_schema to list available tables.', + text: 'Table "not_a_table" does not exist in this save — use pcm_get_save_schema to list available tables.', }); }); @@ -163,6 +163,16 @@ describe("updateSave", () => { "update foo set x = 1", ); }); + + it("accepts a semicolon inside a string literal", () => { + const s = "UPDATE foo SET note = ';'"; + expect(assertWriteStatement(s)).toBe(s); + }); + + it("accepts a WITH … UPDATE CTE (write behind a CTE)", () => { + const s = "WITH x AS (SELECT 1) UPDATE foo SET a = 1"; + expect(assertWriteStatement(s)).toBe(s); + }); }); describe("empty / blank statements", () => { @@ -194,9 +204,9 @@ describe("updateSave", () => { ); }); - it("rejects a WITH … statement", () => { + it("rejects a WITH … SELECT (a read behind a CTE)", () => { expect(() => - assertWriteStatement("WITH x AS (SELECT 1) UPDATE foo SET a = 1"), + assertWriteStatement("WITH x AS (SELECT 1) SELECT * FROM x"), ).toThrowError("Only a single INSERT, UPDATE or DELETE"); }); From 771a1f5cfa7736f536e045683b575abd3a9ed856 Mon Sep 17 00:00:00 2001 From: Mathieu Picciolli Date: Thu, 2 Jul 2026 18:40:48 -0400 Subject: [PATCH 3/8] refactor: simplify statement schema definition in outputSchema --- src/tools/update-save.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/tools/update-save.ts b/src/tools/update-save.ts index a53177b..cf6416c 100644 --- a/src/tools/update-save.ts +++ b/src/tools/update-save.ts @@ -10,9 +10,7 @@ const outputSchema = z.object({ rowsModified: z .number() .describe("Number of rows changed by the statement (INSERT/UPDATE/DELETE)"), - statement: z - .string() - .describe("The normalized statement that was executed"), + statement: z.string().describe("The normalized statement that was executed"), }); export function registerUpdateSave(server: McpServer): void { From cfc87cf7394bbf0f19d9c77fd265db8b8ef9951a Mon Sep 17 00:00:00 2001 From: Mathieu Picciolli Date: Thu, 2 Jul 2026 18:52:48 -0400 Subject: [PATCH 4/8] refactor: simplify default config for withSaveDb function --- src/save-db.ts | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/save-db.ts b/src/save-db.ts index 3d89850..e321744 100644 --- a/src/save-db.ts +++ b/src/save-db.ts @@ -63,9 +63,7 @@ export async function withSaveDb>( fn: (db: SaveDb, save: SaveFile) => T | Promise, config: { queryOnly?: boolean; - } = { - queryOnly: true, - }, + } = {}, ): Promise { let db: SaveDb | undefined; try { @@ -75,7 +73,7 @@ export async function withSaveDb>( const cdbBuffer = await readFile(save.path); db = cdbToSql(cdbBuffer, SQL); - if (config.queryOnly) { + if (config.queryOnly ?? true) { db.run("PRAGMA query_only = ON;"); } From b76ba6736f31647eed2ccb9500713434a93e8c42 Mon Sep 17 00:00:00 2001 From: Mathieu Picciolli Date: Thu, 2 Jul 2026 18:57:53 -0400 Subject: [PATCH 5/8] feat: enhance writeSaveDb function to prevent overwriting existing files and validate output directory --- src/save-db.ts | 49 +++++++++++++++++++++++++++++----- src/tools/update-save.ts | 2 +- test/tools/update-save.test.ts | 41 ++++++++++++++++++++++++++++ 3 files changed, 85 insertions(+), 7 deletions(-) diff --git a/src/save-db.ts b/src/save-db.ts index e321744..6b21c93 100644 --- a/src/save-db.ts +++ b/src/save-db.ts @@ -1,5 +1,5 @@ -import { readFile, writeFile } from "node:fs/promises"; -import { resolve } from "node:path"; +import { readFile, stat, writeFile } from "node:fs/promises"; +import { dirname, resolve } from "node:path"; import type { CallToolResult } from "@modelcontextprotocol/sdk/types.js"; import { cdbToSql, sqlToCdb } from "cdb-converter"; import initSqlJs from "sql.js"; @@ -101,7 +101,8 @@ export async function withSaveDb>( * @param sourcePath - Absolute path of the source save, used only to guard * against overwriting it. * @returns The absolute path written. - * @throws if `outputPath` isn't a `.cdb` file or resolves to `sourcePath`. + * @throws if `outputPath` isn't a `.cdb` file, resolves to `sourcePath`, points + * into a missing directory, or would overwrite an existing file. */ export async function writeSaveDb( db: SaveDb, @@ -111,13 +112,49 @@ export async function writeSaveDb( if (!outputPath.toLowerCase().endsWith(".cdb")) { throw new Error(`Output must be a .cdb file: ${outputPath}`); } - if (resolve(outputPath) === resolve(sourcePath)) { + + const resolvedOutput = resolve(outputPath); + if (resolvedOutput === resolve(sourcePath)) { throw new Error( "outputPath must differ from the source save — the input .cdb is never overwritten.", ); } + // Never clobber an existing file: writes only ever create a new `.cdb`. + if (await pathExists(resolvedOutput)) { + throw new Error( + `outputPath already exists: ${resolvedOutput} — choose a new file name so no existing file is overwritten.`, + ); + } + + // Fail early with an actionable message rather than a raw ENOENT from writeFile. + const parent = dirname(resolvedOutput); + if (!(await isDirectory(parent))) { + throw new Error( + `Output directory does not exist: ${parent} — create it first or point outputPath at an existing directory.`, + ); + } + const cdb = sqlToCdb(db); - await writeFile(outputPath, Buffer.from(cdb)); - return resolve(outputPath); + await writeFile(resolvedOutput, Buffer.from(cdb)); + return resolvedOutput; +} + +/** True if `path` exists (file or directory). */ +async function pathExists(path: string): Promise { + try { + await stat(path); + return true; + } catch { + return false; + } +} + +/** True if `path` exists and is a directory. */ +async function isDirectory(path: string): Promise { + try { + return (await stat(path)).isDirectory(); + } catch { + return false; + } } diff --git a/src/tools/update-save.ts b/src/tools/update-save.ts index cf6416c..2b2a8b3 100644 --- a/src/tools/update-save.ts +++ b/src/tools/update-save.ts @@ -27,7 +27,7 @@ export function registerUpdateSave(server: McpServer): void { outputPath: z .string() .describe( - "Absolute path of the .cdb file to write the modified save to (must differ from savePath)", + "Absolute path of the .cdb file to write the modified save to. Must differ from savePath, sit in an existing directory, and not already exist (existing files are never overwritten).", ), statement: z .string() diff --git a/test/tools/update-save.test.ts b/test/tools/update-save.test.ts index 5fa8c91..307a04b 100644 --- a/test/tools/update-save.test.ts +++ b/test/tools/update-save.test.ts @@ -113,6 +113,47 @@ describe("updateSave", () => { }); }); + it.each( + saveFixtures, + )("refuses to overwrite an existing output file for %s", async (_name, path) => { + const outputPath = join(outDir, "edited.cdb"); + // First write succeeds and creates the file. + const first = await mcp.callTool("pcm_update_save", { + savePath: path, + outputPath, + statement: "UPDATE GAM_config SET gene_i_date = 20991231", + }); + expect(first.isError).toBeUndefined(); + + // A second write to the same path must not clobber it. + const second = await mcp.callTool("pcm_update_save", { + savePath: path, + outputPath, + statement: "UPDATE GAM_config SET gene_i_date = 20991231", + }); + expect(second.isError).toBe(true); + expect(second.content[0]).toEqual({ + type: "text", + text: expect.stringMatching(/already exists/), + }); + }); + + it.each( + saveFixtures, + )("errors when the output directory does not exist for %s", async (_name, path) => { + const result = await mcp.callTool("pcm_update_save", { + savePath: path, + outputPath: join(outDir, "missing", "edited.cdb"), + statement: "UPDATE GAM_config SET gene_i_date = 20991231", + }); + + expect(result.isError).toBe(true); + expect(result.content[0]).toEqual({ + type: "text", + text: expect.stringMatching(/Output directory does not exist/), + }); + }); + it.each( saveFixtures, )("maps a missing table to a schema-discovery hint for %s", async (_name, path) => { From a705175e6fd8b9fc7763d5b05cc8f009e9ccfeca Mon Sep 17 00:00:00 2001 From: Mathieu Picciolli Date: Thu, 2 Jul 2026 19:01:46 -0400 Subject: [PATCH 6/8] Treat only ENOENT as "does not exist" Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- src/save-db.ts | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/save-db.ts b/src/save-db.ts index 6b21c93..089cff2 100644 --- a/src/save-db.ts +++ b/src/save-db.ts @@ -145,8 +145,11 @@ async function pathExists(path: string): Promise { try { await stat(path); return true; - } catch { - return false; + } catch (error) { + if ((error as NodeJS.ErrnoException).code === "ENOENT") { + return false; + } + throw error; } } From 3222034bbc1ddba97386689f40444058502bdc33 Mon Sep 17 00:00:00 2001 From: Mathieu Picciolli Date: Thu, 2 Jul 2026 19:02:06 -0400 Subject: [PATCH 7/8] Treat only ENOENT as "missing" Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- src/save-db.ts | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/save-db.ts b/src/save-db.ts index 089cff2..e1379ca 100644 --- a/src/save-db.ts +++ b/src/save-db.ts @@ -157,7 +157,10 @@ async function pathExists(path: string): Promise { async function isDirectory(path: string): Promise { try { return (await stat(path)).isDirectory(); - } catch { - return false; + } catch (error) { + if ((error as NodeJS.ErrnoException).code === "ENOENT") { + return false; + } + throw error; } } From a6e93753ffbd3b4811d8ace08deaa0c20805e915 Mon Sep 17 00:00:00 2001 From: Mathieu Picciolli Date: Thu, 2 Jul 2026 19:02:59 -0400 Subject: [PATCH 8/8] Use an atomic create Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- src/save-db.ts | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/src/save-db.ts b/src/save-db.ts index e1379ca..f2f2c58 100644 --- a/src/save-db.ts +++ b/src/save-db.ts @@ -136,7 +136,16 @@ export async function writeSaveDb( } const cdb = sqlToCdb(db); - await writeFile(resolvedOutput, Buffer.from(cdb)); + try { + await writeFile(resolvedOutput, Buffer.from(cdb), { flag: "wx" }); + } catch (error) { + if ((error as NodeJS.ErrnoException).code === "EEXIST") { + throw new Error( + `outputPath already exists: ${resolvedOutput} — choose a new file name so no existing file is overwritten.`, + ); + } + throw error; + } return resolvedOutput; }