From 618ba2a682c67339616599531ffef76d9910e972 Mon Sep 17 00:00:00 2001 From: hugo Date: Sat, 28 Feb 2026 16:19:19 +0100 Subject: [PATCH] docs: apply architecture review findings to MCP standalone plan --- MCP_STANDALONE_PLAN.md | 248 ++++++++++++++++++++++++++++------------- 1 file changed, 172 insertions(+), 76 deletions(-) diff --git a/MCP_STANDALONE_PLAN.md b/MCP_STANDALONE_PLAN.md index b6cde7f..206e70b 100644 --- a/MCP_STANDALONE_PLAN.md +++ b/MCP_STANDALONE_PLAN.md @@ -25,7 +25,7 @@ Ship a bundled CLI tool (`bds-mcp`) alongside the Electron app that: │ ScriptEngine │ │ │ db_notifications ← new │ │ TemplateEngine │ │ └────────────────────────────────────┘ │ MCPServer (tools/res) │ │ ▲ -│ ProposalStore (in-memory) │ │ fs.watch(-wal) + 100 ms debounce +│ ProposalStore (in-memory) │ │ chokidar(-wal) + 100 ms debounce └─────────────────────────────┘ │ invalidate engines + IPC to renderer │ agent (Claude Desktop, VS Code…) ─┘ @@ -51,15 +51,16 @@ CREATE TABLE db_notifications ( entity TEXT NOT NULL, -- 'post' | 'media' | 'script' | 'template' entityId TEXT NOT NULL, action TEXT NOT NULL, -- 'created' | 'updated' | 'deleted' - origin TEXT NOT NULL, -- 'cli' | 'app' + fromCli INTEGER NOT NULL DEFAULT 1, -- 1 = written by CLI; reserved for future app→CLI signalling seenAt INTEGER, -- NULL = unprocessed by app createdAt INTEGER NOT NULL ); ``` -The CLI writes a row after every mutation. The app's `NotificationWatcher` polls for -`seenAt IS NULL AND origin = 'cli'`, refreshes the relevant engine cache, emits IPC -events to the renderer, then stamps `seenAt`. +The CLI writes a row after every mutation. The app's `NotificationWatcher` queries for +`seenAt IS NULL AND fromCli = 1`, invalidates the relevant engine cache, emits IPC events +to the renderer, stamps `seenAt`, then **prunes rows whose `seenAt` is older than 1 hour** +to prevent unbounded table growth. --- @@ -102,14 +103,35 @@ Refactor `DatabaseConnection` to accept explicit constructor arguments: interface DatabaseConnectionConfig { dbPath: string; // absolute path to bds.db migrationsFolder: string; // absolute path to the drizzle/ migrations dir - createDirs?: string[]; // extra directories to mkdir at startup + dataDirs?: string[]; // extra directories to mkdir at startup (posts/, media/, etc.) } ``` -The Electron `main.ts` constructs it as today (computing paths via `app.getPath` before -passing them in). The CLI computes the same paths via `platformConfigPath()` and -`__dirname`-relative resolution from the bundle. All Electron imports are removed from -`connection.ts` itself. +Four things must move out of `connection.ts` into `main.ts` (or equivalent callers): + +1. `app.getPath('userData')` in the constructor — replaced by the explicit `dbPath` +2. `app.isPackaged` + `process.resourcesPath` in `runMigrations()` — replaced by + the explicit `migrationsFolder` +3. The `posts/` and `media/` `mkdirSync` calls in the constructor — absorbed into + the new `dataDirs` option (caller supplies the list) +4. **`getDataPaths()`** — this method also calls `app.getPath('userData')` directly and + must be removed from `DatabaseConnection` entirely. Move callers to compute data + paths themselves (they already have `userData` in scope in `main.ts`). + +The Electron `main.ts` constructs it as today, computing paths via `app.getPath` before +passing them in. The CLI uses `platformConfigPath()` and `__dirname`-relative resolution. +All Electron imports are removed from `connection.ts` itself. + +**WAL mode and synchronous pragma** — add to `initializeLocal()` immediately after +creating the client, before running migrations: + +```typescript +await this.localClient.execute('PRAGMA journal_mode=WAL'); +await this.localClient.execute('PRAGMA synchronous=NORMAL'); +``` + +This must be done for **both** the app and CLI connections. It ensures the `-wal` file +exists (enabling chokidar to watch it) and gives consistent write performance. This is a prerequisite for every CLI section that follows. @@ -163,14 +185,17 @@ Engines to update: `PostEngine`, `MediaEngine`, `ScriptEngine`, `TemplateEngine` `src/main/engine/NotificationWatcher.ts` -Uses `fs.watch()` on the SQLite WAL file (`bds.db-wal`) for near-instant change -detection instead of polling. When a write lands in WAL mode, the `-wal` file is modified -before a checkpoint; watching it gives sub-100 ms latency. +Uses **chokidar** (already in `dependencies`) to watch both `bds.db` and `bds.db-wal`. +This gives sub-100 ms latency with zero boilerplate: chokidar handles missing files (the +`-wal` file doesn't exist until the first write after WAL mode is enabled), platform +differences (FSEvents on macOS, inotify on Linux, FSWatch on Windows), and +`add` events for when the WAL file is created for the first time. ```typescript +import chokidar, { FSWatcher } from 'chokidar'; + export class NotificationWatcher { - private dbWatcher: FSWatcher | null = null; - private walWatcher: FSWatcher | null = null; + private watcher: FSWatcher | null = null; private debounceTimer: ReturnType | null = null; constructor( @@ -182,26 +207,20 @@ export class NotificationWatcher { ) {} start(): void { - // Watch the main db file (catches checkpoints) - this.dbWatcher = watch(this.dbPath, { persistent: false }, - () => this.schedule()); - - // Watch the WAL file (catches every write in WAL mode) - // The WAL file may not exist yet if no writes have occurred; we create - // a short-lived retry watcher that tries once per second until the file - // appears, at which point walWatcher is assigned and the retry cancels. - this.attachWalWatcher(); - } - - private attachWalWatcher(): void { - const walPath = this.dbPath + '-wal'; - try { - this.walWatcher = watch(walPath, { persistent: false }, - () => this.schedule()); - } catch { - // -wal does not exist yet; retry in 1 s - setTimeout(() => this.attachWalWatcher(), 1000); - } + // Watch both the main db file (catches checkpoints) and the WAL file + // (catches every write before checkpoint). chokidar handles the case + // where bds.db-wal does not yet exist — it fires 'add' when it appears. + this.watcher = chokidar.watch( + [this.dbPath, `${this.dbPath}-wal`], + { + persistent: false, + ignoreInitial: true, // don't fire on startup for pre-existing files + usePolling: false, // rely on native FSEvents/inotify/kqueue + awaitWriteFinish: false, + }, + ); + this.watcher.on('change', () => this.schedule()); + this.watcher.on('add', () => this.schedule()); // WAL created for first time } private schedule(): void { @@ -215,7 +234,7 @@ export class NotificationWatcher { .from(dbNotifications) .where(and( isNull(dbNotifications.seenAt), - eq(dbNotifications.origin, 'cli'), + eq(dbNotifications.fromCli, 1), )); for (const row of rows) { @@ -229,12 +248,16 @@ export class NotificationWatcher { .set({ seenAt: Date.now() }) .where(eq(dbNotifications.id, row.id)); } + + // Prune rows processed more than 1 hour ago to prevent unbounded growth. + await this.db + .delete(dbNotifications) + .where(lt(dbNotifications.seenAt, Date.now() - 3_600_000)); } stop(): void { if (this.debounceTimer) clearTimeout(this.debounceTimer); - this.dbWatcher?.close(); - this.walWatcher?.close(); + this.watcher?.close().catch(() => {}); } } ``` @@ -257,11 +280,11 @@ async startCli(): Promise { const transport = new StdioServerTransport(); await server.connect(transport); - // Keep alive until the host closes stdin or the process receives a signal + // Block until the MCP host closes stdin (normal session end). + // Signal-based shutdown is the caller's responsibility: do NOT register + // signal handlers here or they will race with the bds-mcp.ts handlers. await new Promise((resolve) => { process.stdin.on('close', resolve); - process.once('SIGTERM', resolve); - process.once('SIGINT', resolve); }); await server.close(); @@ -277,34 +300,55 @@ the single `MCPServer` instance as before. ``` 1. platformConfigPath() → userData dir -2. migrationsFolder = path.join(__dirname, '../drizzle') // bundled alongside .cjs -3. new DatabaseConnection({ dbPath, migrationsFolder }) -4. await db.initializeLocal() -5. SELECT active project (isActive = 1); exit with message if none -6. instantiate engines with CliNotifier DB writer -7. new MCPServer({ postEngine, mediaEngine, … }) -8. await mcpServer.startCli() +2. dbPath = path.join(userData, 'bds.db') +3. migrationsFolder = path.join(__dirname, 'drizzle') // Contents/Resources/drizzle/ +4. new DatabaseConnection({ dbPath, migrationsFolder, dataDirs: [postsDir, mediaDir] }) +5. await db.initializeLocal() // runs WAL + synchronous pragmas, then migrations +6. SELECT active project (isActive = 1); exit with message if none +7. instantiate engines with CliNotifier DB writer +8. new MCPServer({ postEngine, mediaEngine, … }, { proposalTtlMs: 8 * 60 * 60 * 1000 }) +9. await mcpServer.startCli() // blocks until stdin closes +10. await shutdown() // graceful post-stdin exit ``` +`MCPServer` should accept an optional `proposalTtlMs` to override the default 30-minute +TTL. CLI sessions can last hours (overnight agent runs), so **8 hours** is the appropriate +default. Pass `proposalTtlMs` through to the `ProposalStore` constructor. + No `electron` imports. Fully standalone Node process. -**Graceful shutdown** — register handlers *before* `startCli()` so cleanup runs on both -normal exit and forced signals: +**Graceful shutdown** — two paths, no racing: ```typescript -async function shutdown(db: DatabaseConnection, store: ProposalStore): Promise { - store.destroy(); +// Called both by signal handlers (forced) and after normal stdin-close (graceful). +// process.exit() makes it non-reentrant even without the once guards. +async function shutdown(): Promise { + mcpServer.proposalStore.destroy(); await db.close(); // flushes any in-flight @libsql writes process.exit(0); } -process.once('SIGTERM', () => shutdown(db, mcpServer.proposalStore)); -process.once('SIGINT', () => shutdown(db, mcpServer.proposalStore)); -process.once('beforeExit', () => shutdown(db, mcpServer.proposalStore)); +// Signal handlers own interruption; registered before startCli() so they are +// active during the entire session. Do NOT register signals inside startCli(). +process.once('SIGTERM', shutdown); +process.once('SIGINT', shutdown); + +// Normal path: stdin closes → startCli() resolves → server.close() runs → shutdown() +await mcpServer.startCli(); +await shutdown(); ``` -(`startCli()` awaits `process.stdin` close / signal and then falls through to the same -cleanup, so shutdown runs exactly once with the `once` guards.) +When SIGTERM fires, `shutdown()` runs and calls `process.exit(0)` — `startCli()` never +resumes, `server.close()` is skipped (safe: the process is exiting anyway). When stdin +closes cleanly, `startCli()` resumes, `server.close()` runs first, then `shutdown()` +exits. The `once` guards and `process.exit` together make the function non-reentrant. + +**Native module resolution** — When launched as `ELECTRON_RUN_AS_NODE=1`, Electron's +ASAR patching is still active, so `require('@libsql/client')` resolves into +`app.asar/node_modules`. This is a documented side-effect of `ELECTRON_RUN_AS_NODE` that +has been stable since Electron 20. Add an integration test that verifies the module +resolves and the DB opens successfully from the bundled path; if a future Electron major +breaks this, the test will catch it early. --- @@ -319,15 +363,18 @@ target: node, format: cjs, bundle: true The project uses `@libsql/client` with platform-specific native binaries (e.g. `@libsql/darwin-arm64`). These **cannot** be bundled by esbuild. Externalize all -`@libsql/*` packages and ensure the platform binary directories are included alongside -the `.cjs` file in `extraResources`: +`@libsql/*` and `chokidar`-related native packages, and ensure they resolve at runtime +from the app bundle (see the ASAR resolution note in section H): ``` -externals: ['@libsql/client', '@libsql/darwin-arm64', '@libsql/linux-x64-gnu', …] +externals: ['@libsql/client', '@libsql/darwin-arm64', '@libsql/linux-x64-gnu', + 'chokidar', 'fsevents', …] ``` -Also bundle the `drizzle/` migrations folder next to `bds-mcp.cjs` so the path resolution -in section H (`path.join(__dirname, '../drizzle')`) works at runtime inside the app bundle. +The `drizzle/` migrations folder does **not** need to be re-bundled alongside the `.cjs` +file — it is already included in `extraResources` (see J). The path resolution +`path.join(__dirname, 'drizzle')` resolves correctly when `bds-mcp.cjs` sits at +`Contents/Resources/bds-mcp.cjs` and `drizzle/` is at `Contents/Resources/drizzle/`. Runs as part of `npm run build` before the Electron build. @@ -335,18 +382,21 @@ Runs as part of `npm run build` before the Electron build. ### J — electron-builder `extraResources` +The `{ "from": "drizzle", "to": "drizzle" }` entry **already exists** in `package.json`. +Only the CLI bundle itself needs to be added: + ```json "build": { "extraResources": [ - { "from": "dist/cli/bds-mcp.cjs", "to": "bds-mcp.cjs" }, - { "from": "drizzle", "to": "drizzle" } + { "from": "dist/cli/bds-mcp.cjs", "to": "bds-mcp.cjs" } + // { "from": "drizzle", "to": "drizzle" } ← already present, do not duplicate ] } ``` -`bds-mcp.cjs` is placed at `Contents/Resources/bds-mcp.cjs`; the `drizzle/` migrations -folder is placed at `Contents/Resources/drizzle/` so the CLI migration resolver -(`path.join(__dirname, '../drizzle')`) resolves correctly from `Contents/Resources/`. +`bds-mcp.cjs` lands at `Contents/Resources/bds-mcp.cjs`; `drizzle/` is already at +`Contents/Resources/drizzle/`. The CLI migration resolver `path.join(__dirname, 'drizzle')` +resolves `Contents/Resources/drizzle/` correctly from that location. --- @@ -392,6 +442,10 @@ Config file path: - Windows: `%APPDATA%\Claude\claude_desktop_config.json` - Linux: `~/.config/Claude/claude_desktop_config.json` +> **Note:** Verify the current OpenCode config schema before implementing the `opencode` +> entry — the format has changed between releases. Check the live spec at +> https://opencode.ai/docs before writing the `buildEntry` case. + #### Remove from config (new method) ```typescript @@ -450,35 +504,77 @@ proposals lightweight (no DB rows, no filesystem side effects). --- +### N — Renderer: subscribe to `entity:changed` IPC events + +The app's UI must reflect changes the CLI makes while the app is open. `NotificationWatcher` +fires `entity:changed` IPC events (section F), but nothing currently handles them in the +renderer. + +**`preload.ts`** — expose the channel alongside existing IPC listeners: + +```typescript +onEntityChanged: (cb: (payload: EntityChangedPayload) => void) => + ipcRenderer.on('entity:changed', (_, payload) => cb(payload)), +``` + +**Renderer subscription** — in the top-level app component (or a dedicated `IpcListener` +component that mounts once), subscribe on mount and unsubscribe on unmount: + +```typescript +useEffect(() => { + const unsub = window.api.onEntityChanged(({ entity, entityId, action }) => { + // Dispatch store invalidation so the next render fetches fresh data + if (entity === 'post') postsStore.getState().invalidate(entityId); + if (entity === 'media') mediaStore.getState().invalidate(entityId); + if (entity === 'script') scriptsStore.getState().invalidate(entityId); + if (entity === 'template') templatesStore.getState().invalidate(entityId); + }); + return () => unsub(); +}, []); +``` + +Store `invalidate()` methods should clear the cached entry and, if the entity is +currently displayed, trigger an immediate refetch — consistent with how other +IPC-driven refreshes work in the app. + +--- + ## Design Decisions | Question | Decision | |---|---| | Active project in CLI | Use `isActive = 1` from DB; fail fast if none | | Proposal durability | Scripts/templates use DB draft rows; metadata proposals stay in-memory (failure mode documented in M) | +| Proposal TTL | 30 min default in app; **8 hours** in CLI (overnight agent sessions) | | App View tool calls in CLI mode | `app.callServerTool()` routes through the MCP host, not a direct HTTP call — no secondary port needed | -| Claude Desktop support | stdio via bundled `bds-mcp.cjs` + `ELECTRON_RUN_AS_NODE=1` | +| Claude Desktop support | stdio via bundled `bds-mcp.cjs` + `ELECTRON_RUN_AS_NODE=1` (entitlements already set) | | Remove from config | New `removeFromConfig()` method + Remove buttons in UI | -| DB change detection | `fs.watch()` on `-wal` file + 100 ms debounce; falls back to watching main db file on checkpoint | -| DB coupling to Electron | `DatabaseConnection` accepts explicit paths; Electron-specific path resolution stays in `main.ts` | +| DB change detection | **chokidar** on both `bds.db` and `bds.db-wal`; 100 ms debounce; handles missing WAL via `add` events | +| WAL mode | Explicit `PRAGMA journal_mode=WAL; PRAGMA synchronous=NORMAL` in `initializeLocal()` for both app and CLI | +| `db_notifications` growth | Rows pruned after `seenAt` is 1 hour old, inside each `process()` call | +| DB coupling to Electron | `DatabaseConnection` accepts explicit paths; `getDataPaths()` removed; Electron path resolution stays in `main.ts` | +| Native module resolution | `ELECTRON_RUN_AS_NODE` inherits Electron ASAR require-hooking; integration test guards against regression | +| Signal handling | Signal handlers in `bds-mcp.ts` only; `startCli()` only listens to stdin-close — no competing handlers | +| Renderer live updates | Section N: `entity:changed` IPC → store `invalidate()` → refetch | --- ## Implementation Order (TDD per AGENTS.md) 1. **Migrations** (A + B) — schema changes first, all other work builds on them -2. **Decouple `DatabaseConnection` from Electron** (C0) — prerequisite for all CLI code; update existing DB tests to pass explicit paths +2. **Decouple `DatabaseConnection` from Electron** (C0) — remove `app` import, move `getDataPaths()` to callers, add WAL pragmas; update existing DB tests to pass explicit paths 3. **`platformConfigPath()`** (C) — pure function, easy to test 4. **`CliNotifier` + DB writer** (D) — unit tests with mock DB 5. **Engine `invalidate()` API** (E) — add and test on each engine before NotificationWatcher 6. **`ScriptEngine`/`TemplateEngine` draft lifecycle** (L) — tests first -7. **`ProposalStore` DB mapping** (M) — extend existing tests -8. **`NotificationWatcher`** (F) — test with mock DB rows + mock `fs.watch` emitter -9. **`MCPAgentConfigEngine` — `claude-desktop` + `removeFromConfig`** (K) — extend existing tests +7. **`ProposalStore` TTL option + DB mapping** (M) — extend existing tests; add `proposalTtlMs` constructor arg +8. **`NotificationWatcher`** (F) — test with mock DB rows + mock chokidar emitter (emit `'change'` / `'add'` events on a fake `FSWatcher`) +9. **`MCPAgentConfigEngine` — `claude-desktop` + `removeFromConfig`** (K) — extend existing tests; verify opencode format before writing its case 10. **`MCPServer.startCli()`** (G) — test with in-process `StdioServerTransport` -11. **`src/cli/bds-mcp.ts`** (H) — integration test: spawn process, send `initialize`, assert response, assert clean shutdown on SIGTERM -12. **Build target + `extraResources`** (I + J) — verify `npm run build` produces bundle and `drizzle/` is co-located +11. **`src/cli/bds-mcp.ts`** (H) — integration test: spawn process, send `initialize`, assert response, assert clean shutdown on SIGTERM and on stdin-close +12. **Build target + `extraResources`** (I + J) — verify `npm run build` produces `dist/cli/bds-mcp.cjs`; `drizzle/` entry already in `extraResources`, only add `bds-mcp.cjs` 13. **UI: Remove buttons** — renderer component changes, follows K 14. **`NotificationWatcher` wired in `main.ts`** (F, app side) + stopped in `before-quit` +15. **Renderer `entity:changed` subscription** (N) — `preload.ts` exposure + store `invalidate()` hookup + component subscription Each step: write failing test → implement → green → next.