From 47baf73a8bd9cfce0450b65e965ee67f77309e9e Mon Sep 17 00:00:00 2001 From: hongming Date: Thu, 28 May 2026 17:17:25 -0700 Subject: [PATCH 1/2] fix(session-cursor): trySave() + set() empty-guard + temp-mode hardening (v1.4.1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Non-blocking review follow-ups from molecule-mcp-server#30: - trySave(onError?): never-throwing save() wrapper for poll-tick callers, so each adapter doesn't re-implement the try/catch (SSOT for the next adapter). - set() rejects empty/non-string activityId, matching load()'s filter so the in-memory state can't diverge from what survives a save→load round-trip. - save() clears a stale same-PID temp before writing: writeFileSync only applies `mode` on create, so writing over a leftover temp could leak a 0o644 mode through the rename. Now always a fresh 0o600 create. Additive; tests added for each. v1.4.1. Co-Authored-By: Claude Opus 4.8 (1M context) --- package-lock.json | 8 +++--- package.json | 2 +- src/__tests__/session-cursor.test.ts | 38 +++++++++++++++++++++++++++- src/session-cursor.ts | 33 +++++++++++++++++++++++- 4 files changed, 74 insertions(+), 7 deletions(-) diff --git a/package-lock.json b/package-lock.json index 9d68f4a..7b3087d 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "@molecule-ai/mcp-server", - "version": "1.4.0", + "version": "1.4.1", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "@molecule-ai/mcp-server", - "version": "1.4.0", + "version": "1.4.1", "dependencies": { "@modelcontextprotocol/sdk": "^1.12.0", "pino": "^9.6.0", @@ -4077,7 +4077,7 @@ } }, "node_modules/natural-compare": { - "version": "1.4.0", + "version": "1.4.1", "resolved": "https://registry.npmjs.org/natural-compare/-/natural-compare-1.4.0.tgz", "integrity": "sha512-OWND8ei3VtNC9h7V60qff3SVobHr996CTwgxubgyQYEpg290h9J0buyECNNJexkFm5sOajh5G116RYA1c8ZMSw==", "dev": true, @@ -4178,7 +4178,7 @@ } }, "node_modules/once": { - "version": "1.4.0", + "version": "1.4.1", "resolved": "https://registry.npmjs.org/once/-/once-1.4.0.tgz", "integrity": "sha512-lNaJgI+2Q5URQBkccEKHTQOPaXdUxnZZElQTZY0MFUAuaEqe1E+Nyvgdz/aIyNi6Z9MzO5dv1H8n58/GELp3+w==", "license": "ISC", diff --git a/package.json b/package.json index 6bb0976..e31d7f2 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@molecule-ai/mcp-server", - "version": "1.4.0", + "version": "1.4.1", "description": "MCP server for Molecule AI Agent Team \u2014 manage workspaces, agents, and skills from any AI coding tool", "type": "module", "exports": { diff --git a/src/__tests__/session-cursor.test.ts b/src/__tests__/session-cursor.test.ts index 6d4ff23..2809b0a 100644 --- a/src/__tests__/session-cursor.test.ts +++ b/src/__tests__/session-cursor.test.ts @@ -1,4 +1,4 @@ -import { mkdtempSync, rmSync, writeFileSync, readFileSync, readdirSync, existsSync } from "node:fs"; +import { mkdtempSync, rmSync, writeFileSync, readFileSync, readdirSync, existsSync, statSync, chmodSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; @@ -126,6 +126,42 @@ describe("CursorStore", () => { expect(JSON.parse(readFileSync(join(dir, "cursor.json"), "utf8"))).toEqual({ "ws-1": "act-100" }); }); + it("set rejects empty / non-string so the round-trip stays total", () => { + const a = new CursorStore({ stateDir: dir }); + expect(() => a.set("ws-1", "")).toThrow(); + // @ts-expect-error — guarding the JS-caller path that bypasses the type + expect(() => a.set("ws-1", undefined)).toThrow(); + expect(a.size).toBe(0); // nothing stored + a.set("ws-1", "act-1"); + a.save(); + expect(new CursorStore({ stateDir: dir }).load().get("ws-1")).toBe("act-1"); + }); + + it("trySave returns true on success and false+onError on failure", () => { + const ok = new CursorStore({ stateDir: dir }); + ok.set("ws-1", "act-1"); + const errs: unknown[] = []; + expect(ok.trySave((e) => errs.push(e))).toBe(true); + expect(errs).toHaveLength(0); + + // stateDir under a non-existent parent → writeFileSync throws → trySave false. + const bad = new CursorStore({ stateDir: join(dir, "nope", "deeper") }); + bad.set("ws-1", "act-1"); + expect(bad.trySave((e) => errs.push(e))).toBe(false); + expect(errs).toHaveLength(1); + }); + + it("save applies fileMode even over a stale same-PID temp (no 0o644 leak)", () => { + const a = new CursorStore({ stateDir: dir }); // default mode 0o600 + // Simulate a crashed prior save leaving a world-readable temp. + const tmp = join(dir, `cursor.json.tmp.${process.pid}`); + writeFileSync(tmp, "{}", { mode: 0o644 }); + chmodSync(tmp, 0o644); + a.set("ws-1", "act-1"); + a.save(); + expect(statSync(join(dir, "cursor.json")).mode & 0o777).toBe(0o600); + }); + it("unlink removes the backing file and is a no-op when already gone", () => { const a = new CursorStore({ stateDir: dir, sessionKey: "777" }); a.set("ws-1", "act-1"); diff --git a/src/session-cursor.ts b/src/session-cursor.ts index 6e8c9fc..3ff2027 100644 --- a/src/session-cursor.ts +++ b/src/session-cursor.ts @@ -175,6 +175,13 @@ export class CursorStore { } set(workspaceId: string, activityId: string): void { + // Reject empty/non-string so the in-memory state can't diverge from what + // survives a save→load round-trip (load() drops empty/non-string values). + if (typeof activityId !== "string" || activityId.length === 0) { + throw new Error( + `CursorStore.set: activityId must be a non-empty string (workspace ${workspaceId})`, + ); + } this.cursors.set(workspaceId, activityId); } @@ -193,16 +200,40 @@ export class CursorStore { /** * Atomically persist to disk (temp + rename). The temp name is PID-suffixed * so two writers never collide on the temp path. Throws on write failure — - * the caller (typically a setInterval tick) decides whether to log+swallow. + * the caller decides whether to log+swallow (or use {@link trySave}). */ save(): void { const obj: Record = {}; for (const [k, v] of this.cursors) obj[k] = v; const tmp = `${this.path}.tmp.${process.pid}`; + // Clear any leftover temp from a crashed same-PID save first: writeFileSync + // only applies `mode` when it CREATES the file, so writing over a stale temp + // would silently inherit that file's mode (a 0o644 leak through rename). + try { + unlinkSync(tmp); + } catch { + // No stale temp — the common case. + } writeFileSync(tmp, JSON.stringify(obj, null, 2), { mode: this.fileMode }); renameSync(tmp, this.path); } + /** + * {@link save} wrapped to never throw — for poll-tick / setInterval callers + * where an unhandled rejection would kill the loop. Returns true on success; + * on failure invokes `onError` (if given) and returns false. Adapters should + * prefer this over hand-rolling the try/catch. + */ + trySave(onError?: (err: unknown) => void): boolean { + try { + this.save(); + return true; + } catch (err) { + onError?.(err); + return false; + } + } + /** Remove the backing file. Used by a secondary session on clean exit. No-op if already gone. */ unlink(): void { try { -- 2.52.0 From ead09d0fdf014148b30e9443ca76bf47759df361 Mon Sep 17 00:00:00 2001 From: hongming Date: Thu, 28 May 2026 17:24:42 -0700 Subject: [PATCH 2/2] review fixes: skip-empty set() (no throw) + correct lockfile bump MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adversarial review of this PR found two issues: - The 1.4.0→1.4.1 version bump was a blind text replace that also flipped the `version` of two unrelated transitive deps (natural-compare, once) to a nonexistent 1.4.1, leaving the lockfile internally inconsistent. Redone as a precise JSON edit of only the root self-version. - set() throwing on empty/non-string, combined with the channel's `void pollWorkspace` call site, could turn a (platform-pathological) empty activity id into a stuck-cursor replay loop. Changed to a silent no-op skip: same round-trip-consistency goal, no exception/abort path. --- package-lock.json | 4 ++-- src/__tests__/session-cursor.test.ts | 8 ++++---- src/session-cursor.ts | 11 +++++------ 3 files changed, 11 insertions(+), 12 deletions(-) diff --git a/package-lock.json b/package-lock.json index 7b3087d..dd14a4a 100644 --- a/package-lock.json +++ b/package-lock.json @@ -4077,7 +4077,7 @@ } }, "node_modules/natural-compare": { - "version": "1.4.1", + "version": "1.4.0", "resolved": "https://registry.npmjs.org/natural-compare/-/natural-compare-1.4.0.tgz", "integrity": "sha512-OWND8ei3VtNC9h7V60qff3SVobHr996CTwgxubgyQYEpg290h9J0buyECNNJexkFm5sOajh5G116RYA1c8ZMSw==", "dev": true, @@ -4178,7 +4178,7 @@ } }, "node_modules/once": { - "version": "1.4.1", + "version": "1.4.0", "resolved": "https://registry.npmjs.org/once/-/once-1.4.0.tgz", "integrity": "sha512-lNaJgI+2Q5URQBkccEKHTQOPaXdUxnZZElQTZY0MFUAuaEqe1E+Nyvgdz/aIyNi6Z9MzO5dv1H8n58/GELp3+w==", "license": "ISC", diff --git a/src/__tests__/session-cursor.test.ts b/src/__tests__/session-cursor.test.ts index 2809b0a..c9a083e 100644 --- a/src/__tests__/session-cursor.test.ts +++ b/src/__tests__/session-cursor.test.ts @@ -126,11 +126,11 @@ describe("CursorStore", () => { expect(JSON.parse(readFileSync(join(dir, "cursor.json"), "utf8"))).toEqual({ "ws-1": "act-100" }); }); - it("set rejects empty / non-string so the round-trip stays total", () => { + it("set ignores empty / non-string so the round-trip stays total (no throw)", () => { const a = new CursorStore({ stateDir: dir }); - expect(() => a.set("ws-1", "")).toThrow(); - // @ts-expect-error — guarding the JS-caller path that bypasses the type - expect(() => a.set("ws-1", undefined)).toThrow(); + a.set("ws-1", ""); // no-op, never throws + // @ts-expect-error — JS-caller path that bypasses the type + a.set("ws-1", undefined); expect(a.size).toBe(0); // nothing stored a.set("ws-1", "act-1"); a.save(); diff --git a/src/session-cursor.ts b/src/session-cursor.ts index 3ff2027..2bb3644 100644 --- a/src/session-cursor.ts +++ b/src/session-cursor.ts @@ -175,13 +175,12 @@ export class CursorStore { } set(workspaceId: string, activityId: string): void { - // Reject empty/non-string so the in-memory state can't diverge from what + // Ignore empty/non-string so the in-memory state can't diverge from what // survives a save→load round-trip (load() drops empty/non-string values). - if (typeof activityId !== "string" || activityId.length === 0) { - throw new Error( - `CursorStore.set: activityId must be a non-empty string (workspace ${workspaceId})`, - ); - } + // A no-op (not a throw): callers like a poll tick advance the cursor via + // `set(ws, newest)` from a `void`-launched loop, where a throw would abort + // the tick before its save() and could wedge the cursor — a skip can't. + if (typeof activityId !== "string" || activityId.length === 0) return; this.cursors.set(workspaceId, activityId); } -- 2.52.0