From 40a75d342f2467a647c21e9e76d9cd550fd292c9 Mon Sep 17 00:00:00 2001 From: smartass Date: Thu, 11 Jun 2026 22:50:02 +0500 Subject: [PATCH] docs: apply Codex review round 1 Accepted: dep major ranges, ssh client leak fix, tmp-name hardening, dispose-ordering note, invalidate-during-build test, testable parseArgs, coverage justification, README security sections. Rejected with evidence: MySQL session read-only blocks DML/DDL (verified on mysql:8.4, ERROR 1792); in-process store races impossible (synchronous read-modify-write). --- .../plans/2026-06-11-dbmole-mcp.md | 132 +++++++++++++++--- .../specs/2026-06-11-dbmole-mcp-design.md | 26 ++-- 2 files changed, 127 insertions(+), 31 deletions(-) diff --git a/docs/superpowers/plans/2026-06-11-dbmole-mcp.md b/docs/superpowers/plans/2026-06-11-dbmole-mcp.md index e34111f..35a1df6 100644 --- a/docs/superpowers/plans/2026-06-11-dbmole-mcp.md +++ b/docs/superpowers/plans/2026-06-11-dbmole-mcp.md @@ -55,14 +55,19 @@ } ``` -- [ ] **Step 2: Install dependencies (latest versions)** +- [ ] **Step 2: Install dependencies (pinned major ranges)** ```bash -npm install @modelcontextprotocol/server pg mysql2 ssh2 zod -npm install -D @modelcontextprotocol/client @biomejs/biome @types/node @types/pg @types/ssh2 @vitest/coverage-v8 testcontainers @testcontainers/postgresql @testcontainers/mysql tsup tsx typescript vitest +npm install @modelcontextprotocol/server@^2 pg@^8 mysql2@^3 ssh2@^1 zod@^4 +npm install -D @modelcontextprotocol/client@^2 @biomejs/biome@^2 @types/node@^22 @types/pg@^8 @types/ssh2@^1 @vitest/coverage-v8@^3 testcontainers@^11 @testcontainers/postgresql@^11 @testcontainers/mysql@^11 tsup@^8 tsx@^4 typescript@^5 vitest@^3 ``` Expected: both commands exit 0, `package.json` gains dependencies/devDependencies. +The committed `package-lock.json` is the exact pin. The majors carry API shape: +MCP SDK v2 split packages (`@modelcontextprotocol/server`/`client` per the official +migration guide), zod 4 (`zod/v4` import), testcontainers 11, vitest 3 +(`test.projects`). If a range fails to resolve, run `npm view versions`, +then stop and re-verify the affected APIs against that package's docs before adapting. - [ ] **Step 3: Create tsconfig.json** @@ -151,7 +156,7 @@ export default defineConfig({ }) ``` -`src/index.ts` is process wiring (signals, stdio, process.exit) — it is exercised by hand/Docker, excluded from unit coverage. +Coverage policy: thresholds gate the unit project only — integration tests need Docker and act as a separate pass/fail gate (`npm run test:int`, Task 21), so CI without Docker still enforces the 90% bar. `src/index.ts` is excluded: it is pure process wiring (signals, stdio transport, `process.exit`) with no logic left after `parseArgs` moves to `src/cli.ts` (Task 16); it is exercised by the JSON-RPC smoke checks in Tasks 16 and 20. - [ ] **Step 6: Create tsup.config.ts** @@ -634,9 +639,12 @@ export const readStore = (path: string): ConnectionConfig[] => { return parseConnections(parsed, `store ${path}`) } +let writeSeq = 0 + export const writeStore = (path: string, connections: ConnectionConfig[]): void => { mkdirSync(dirname(path), { recursive: true, mode: 0o700 }) - const tmp = `${path}.${process.pid}.tmp` + writeSeq += 1 + const tmp = `${path}.${process.pid}.${writeSeq}.tmp` writeFileSync(tmp, `${JSON.stringify(connections, null, 4)}\n`, { mode: 0o600 }) renameSync(tmp, path) } @@ -644,6 +652,8 @@ export const writeStore = (path: string, connections: ConnectionConfig[]): void Atomicity: write to a same-directory tmp file, then `rename` (atomic on POSIX). `writeFileSync` mode applies because the tmp file is always new; rename preserves it. +Concurrency model (matches the spec): the whole read-modify-write path is synchronous — no await points — so the Node event loop serializes mutations within a process; the tmp name cannot be reused mid-write. `writeSeq` is defense-in-depth in case this code ever becomes async. Across processes tmp names differ by pid and rename is last-write-wins, which the spec accepts explicitly. + - [ ] **Step 4: Run test to verify it passes** Run: `npx vitest run --project unit test/unit/config/store.test.ts` @@ -1380,10 +1390,15 @@ export const openTunnel = async ( ) }) - await new Promise((resolve, reject) => { - server.once('error', reject) - server.listen(0, '127.0.0.1', resolve) - }) + try { + await new Promise((resolve, reject) => { + server.once('error', reject) + server.listen(0, '127.0.0.1', resolve) + }) + } catch (error) { + client.end() + throw error + } const address = server.address() as net.AddressInfo @@ -2485,7 +2500,7 @@ import { beforeEach, describe, expect, it, vi } from 'vitest' import { createManager } from '../../../src/db/manager.js' import type { Registry } from '../../../src/config/registry.js' import type { Driver, DriverTarget } from '../../../src/db/driver.js' -import type { ConnectionConfig, ResolvedConnection } from '../../../src/config/types.js' +import type { ConnectionConfig, ResolvedConnection, SshConfig } from '../../../src/config/types.js' import type { Tunnel } from '../../../src/net/tunnel.js' const config = (extra: Partial = {}): ConnectionConfig => ({ @@ -2638,6 +2653,32 @@ describe('createManager', () => { await expect(manager.invalidate('ghost')).resolves.toBeUndefined() }) + it('invalidate during an in-flight build disposes the entry once built', async () => { + let release: () => void = () => {} + const gate = new Promise(resolve => { + release = resolve + }) + const gatedTunnel = vi.fn(async (ssh: SshConfig, host: string, port: number) => { + await gate + return createTunnel(ssh, host, port) + }) + resolved = { + config: config({ ssh: { host: 'bastion', port: 22, user: 'root', password: 'x' } }), + source: 'store', + hash: 'ssh1' + } + const manager = createManager(registry, { createDriver, createTunnel: gatedTunnel }) + const pending = manager.get('c') + const invalidated = manager.invalidate('c') + release() + await pending + await invalidated + expect(drivers[0].disposed).toBe(true) + expect(tunnels[0].closed).toBe(true) + const rebuilt = await manager.get('c') + expect(rebuilt.driver).toBe(drivers[1]) + }) + it('disposeAll disposes everything', async () => { const manager = createManager(registry, { createDriver, createTunnel }) await manager.get('c') @@ -2702,6 +2743,9 @@ export const createManager = (registry: Registry, deps: ManagerDeps = {}): Manag const cache = new Map() const disposeEntry = async (entry: Entry): Promise => { + // Order matters: pg/mysql2 pool.end() waits for in-flight queries to + // finish and clients to be released, so the driver drains BEFORE the + // tunnel closes. New work lands on the rebuilt entry, not this one. await entry.driver.dispose().catch(() => {}) await entry.tunnel?.close().catch(() => {}) } @@ -3568,12 +3612,12 @@ git commit -m "feat: list_databases, list_tables and describe_table tools" --- -### Task 16: Server assembly + entry point (`src/server.ts`, `src/index.ts`) +### Task 16: Server assembly + entry point (`src/server.ts`, `src/cli.ts`, `src/index.ts`) **Files:** -- Create: `src/server.ts` +- Create: `src/server.ts`, `src/cli.ts` - Modify: `src/index.ts` (replace stub) -- Test: `test/unit/server.test.ts` +- Test: `test/unit/server.test.ts`, `test/unit/cli.test.ts` - Delete: `test/unit/smoke.test.ts` - [ ] **Step 1: Write the failing test** @@ -3646,25 +3690,60 @@ export const createServer = (registry: Registry, manager: Manager): McpServer => } ``` -- [ ] **Step 4: Replace src/index.ts stub, delete smoke test** +- [ ] **Step 4: Write src/cli.ts with its test, replace src/index.ts stub, delete smoke test** + +`src/cli.ts` (extracted from the entry point so argument parsing is unit-testable): + +```ts +export const parseArgs = ( + argv: string[], + env: NodeJS.ProcessEnv = process.env +): { configPath?: string } => { + const flagIndex = argv.indexOf('--config') + if (flagIndex !== -1 && argv[flagIndex + 1]) { + return { configPath: argv[flagIndex + 1] } + } + return { configPath: env.DBMOLE_CONFIG } +} +``` + +`test/unit/cli.test.ts`: + +```ts +import { describe, expect, it } from 'vitest' +import { parseArgs } from '../../src/cli.js' + +describe('parseArgs', () => { + it('prefers the --config flag', () => { + expect(parseArgs(['--config', '/tmp/c.json'], { DBMOLE_CONFIG: '/env.json' })).toEqual({ + configPath: '/tmp/c.json' + }) + }) + + it('falls back to DBMOLE_CONFIG env', () => { + expect(parseArgs([], { DBMOLE_CONFIG: '/env.json' })).toEqual({ configPath: '/env.json' }) + }) + + it('returns undefined without flag or env', () => { + expect(parseArgs([], {})).toEqual({ configPath: undefined }) + }) + + it('ignores --config without a value', () => { + expect(parseArgs(['--config'], {})).toEqual({ configPath: undefined }) + }) +}) +``` `src/index.ts`: ```ts import { StdioServerTransport } from '@modelcontextprotocol/server/stdio' +import { parseArgs } from './cli.js' import { createRegistry } from './config/registry.js' import { defaultStorePath } from './config/store.js' import { createManager } from './db/manager.js' import { createServer } from './server.js' -const parseArgs = (argv: string[]): { configPath?: string } => { - const flagIndex = argv.indexOf('--config') - if (flagIndex !== -1 && argv[flagIndex + 1]) { - return { configPath: argv[flagIndex + 1] } - } - return { configPath: process.env.DBMOLE_CONFIG } -} - const main = async () => { const { configPath } = parseArgs(process.argv.slice(2)) const registry = createRegistry({ storePath: defaultStorePath(), configPath }) @@ -4191,7 +4270,8 @@ Content requirements (write actual prose, in English): 3. **Connection sources cascade** — table of the three layers (env `DBMOLE_CONNECTIONS` JSON array > config file via `--config`/`DBMOLE_CONFIG` with `{ "connections": [...] }` > store `~/.config/dbmole/connections.json`, override path with `DBMOLE_STORE`), name-shadowing rule, store mutability via tools. 4. **Connection config reference** — full field table including the `ssh` block and `readonly` flag, with one full JSON example (postgres behind ssh with privateKeyPath). 5. **Tools** — table of all nine tools with one-line descriptions. -6. **Readonly mode** — what it does (server-enforced read-only sessions), what it does not protect against (explicit SET by a malicious caller). +6. **Readonly mode** — what it does (server-enforced read-only sessions: pg via `default_transaction_read_only` startup option, mysql via `SET SESSION TRANSACTION READ ONLY`; both block DML and DDL — verified against postgres:17 and mysql:8.4 in the integration suite), what it does not protect against (a caller can issue an explicit `SET ... READ WRITE`), and the hard-guarantee recommendation: use a database user with read-only privileges for untrusted contexts. +6a. **Security notes** — connections (including passwords and ssh credentials) are stored in plaintext at `~/.config/dbmole/connections.json` with `0600` permissions, same trust model as `~/.pgpass`; anyone who can read the file owns the credentials. For secrets you don't want on disk, supply them via `DBMOLE_CONNECTIONS` env or the `--config` file instead. Database error messages (codes, hints, object names) are returned to the MCP client by design — the caller is the database client. 7. **Docker** — build and run: ```bash @@ -4258,3 +4338,9 @@ git commit -m "chore: final cleanup" # only if there are changes - Spec coverage: cascade (Task 6), hot reload (Tasks 6/11), ssh tunnels (Tasks 7/19), readonly enforcement (Tasks 9/10, verified in 17/18), 9 tools (Tasks 13-16), npm bin + Docker (Tasks 1/20), coverage gate (Task 21). `update_connection` null-removal semantics: Tasks 6/13. Single-statement rule: Task 9 (pg guard) + Task 10 (`multipleStatements: false`). - Known risk points called out inline: `InMemoryTransport` import location (Task 12), sshd wait-strategy log line (Task 19), Biome schema version (Task 1). + +## Codex review round (applied) + +Accepted and folded in: dependency major ranges (Task 1), ssh client leak on listen failure (Task 7), `writeSeq` tmp-name hardening + concurrency-model note (Task 4), dispose-ordering comment and invalidate-during-build test (Task 11), `parseArgs` extracted to testable `src/cli.ts` (Task 16), unit-only coverage justification (Task 1), README security/readonly sections (Task 20). Spec updated to name the SDK v2 packages and to state the atomicity and readonly mechanics precisely. + +Rejected with evidence: "MySQL `SET SESSION TRANSACTION READ ONLY` does not block autocommit DML/DDL" — disproven empirically on mysql:8.4 (INSERT/UPDATE/CREATE TABLE all fail with ERROR 1792, SELECT works), so the readonly integration test in Task 18 is valid as written. In-process store-write races are impossible: the read-modify-write path is fully synchronous. Cross-process last-write-wins is an accepted, documented trade-off. diff --git a/docs/superpowers/specs/2026-06-11-dbmole-mcp-design.md b/docs/superpowers/specs/2026-06-11-dbmole-mcp-design.md index c1fb357..2a8bdb7 100644 --- a/docs/superpowers/specs/2026-06-11-dbmole-mcp-design.md +++ b/docs/superpowers/specs/2026-06-11-dbmole-mcp-design.md @@ -8,7 +8,8 @@ MCP-сервер (stdio) для работы с PostgreSQL и MySQL через ## Стек - TypeScript, ESM, Node >= 20 -- `@modelcontextprotocol/sdk` — McpServer + StdioServerTransport +- `@modelcontextprotocol/server` (MCP SDK v2) — McpServer + StdioServerTransport; + `@modelcontextprotocol/client` в devDeps для тестов через InMemoryTransport - `zod` — схемы входов тулзов и валидация конфигов - `pg` — драйвер PostgreSQL, `mysql2` — драйвер MySQL - `ssh2` — SSH-туннели (pure JS, не требует ssh-бинаря в Docker) @@ -33,8 +34,11 @@ MCP-сервер (stdio) для работы с PostgreSQL и MySQL через - Стор перечитывается при каждом обращении (resolve, list, mutate) — без кеширования файла. Несколько одновременных инстансов dbmole видят изменения друг друга сразу. -- Запись стора атомарная: tmp-файл + rename. Перед каждой мутацией — re-read - (read-modify-write), последняя запись побеждает. +- Атомарность — на уровне подмены файла: tmp-файл + rename (атомарен на POSIX). + Внутри процесса мутации сериализованы синхронным выполнением (в read-modify-write + нет await-точек, event loop не прерывает его). Между процессами остаётся окно + check-then-write — принято осознанно: последняя запись побеждает, конфликт + означает потерю одной мутации и чинится повторным add/update. - У каждого подключения в выдаче `list_connections` есть `source: env | config | store`. - `update/remove_connection` по записи из env/config → ошибка с указанием источника. - `add_connection` с именем, занятым любым слоем, → ошибка (никаких теневых записей). @@ -108,13 +112,19 @@ src/ ## Readonly -Серверное принуждение, не SQL-парсинг. На каждом новом коннекте пула: +Серверное принуждение, не SQL-парсинг: -- pg: `SET default_transaction_read_only = on` -- mysql: `SET SESSION TRANSACTION READ ONLY` +- pg: startup-параметр libpq `options='-c default_transaction_read_only=on'` — + сессия рождается read-only, гонки с выдачей коннекта из пула исключены +- mysql: `SET SESSION TRANSACTION READ ONLY` при первом checkout каждого + физического соединения пула (у mysql2 нет init-SQL опции в конфиге пула) -Блокирует DML и DDL на стороне движка. Обходится только явным `SET` в SQL — -для агентского сценария приемлемо (защита от случайной записи, не от злого умысла). +Блокирует DML и DDL на стороне движка. Проверено эмпирически на mysql:8.4: +INSERT/UPDATE/CREATE TABLE под session read-only падают с ERROR 1792 +«Cannot execute statement in a READ ONLY transaction», SELECT работает. +Обходится только явным `SET` в SQL — это защита от случайной записи, не от +злого умысла. Жёсткая гарантия — read-only пользователь БД; рекомендация +фиксируется в README. ## Тулзы