Skip to content

Commit 240ce80

Browse files
committed
fix(mcp): address code review — atomic config write, resilient emit chain, ordering comment, sync test
- writeJsonConfig: write to a sibling .clerk-tmp-<pid> file then rename so concurrent readers (e.g. Claude Code) never see a partial ~/.claude.json - emit write chain: catch EPIPE / write errors so a single failure doesn't wedge every subsequent frame - forwardJsonBody: note that protocol-version capture ordering relies on the MCP client awaiting the initialize reply before sending the next request - GLOBAL_FLAGS_WITH_VALUE: export the set and add a test that fails if a new root value-option in cli-program.ts is not listed there
1 parent 7fb87a6 commit 240ce80

4 files changed

Lines changed: 40 additions & 6 deletions

File tree

packages/cli-core/src/commands/mcp/clients/json-config.ts

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@
99
* formatting and a 2-space indent.
1010
*/
1111

12-
import { chmod, mkdir } from "node:fs/promises";
12+
import { chmod, mkdir, rename, unlink, writeFile } from "node:fs/promises";
1313
import { dirname } from "node:path";
1414
import { log } from "../../../lib/log.ts";
1515
import { CliError, ERROR_CODE, errorMessage } from "../../../lib/errors.ts";
@@ -53,8 +53,18 @@ export async function readJsonConfig(path: string): Promise<ConfigRecord> {
5353

5454
export async function writeJsonConfig(path: string, config: ConfigRecord): Promise<void> {
5555
log.debug(`mcp: write ${path}`);
56-
await mkdir(dirname(path), { recursive: true, mode: 0o700 });
57-
await Bun.write(path, JSON.stringify(config, null, 2) + "\n");
56+
const dir = dirname(path);
57+
await mkdir(dir, { recursive: true, mode: 0o700 });
58+
// Atomic write: write to a sibling temp file then rename so a concurrent
59+
// reader (e.g. Claude Code) never sees a partial file.
60+
const tmp = `${path}.clerk-tmp-${process.pid}`;
61+
try {
62+
await writeFile(tmp, JSON.stringify(config, null, 2) + "\n", { mode: 0o600 });
63+
await rename(tmp, path);
64+
} catch (error) {
65+
await unlink(tmp).catch(() => {});
66+
throw error;
67+
}
5868
await restrictPermissions(path);
5969
}
6070

packages/cli-core/src/commands/mcp/run.ts

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,12 @@ export async function mcpRun(options: McpOptions = {}, streams: RunStreams = {})
5050
const emit: Emit = (message) => {
5151
captureProtocolVersion(message, session);
5252
const line = JSON.stringify(message) + "\n";
53-
writeTail = writeTail.then(() => writeRaw(line));
53+
// Swallow write errors (e.g. EPIPE) so one failed frame doesn't wedge the chain.
54+
writeTail = writeTail
55+
.then(() => writeRaw(line))
56+
.catch((err: unknown) => {
57+
log.debug(`mcp run: write error — ${errorMessage(err)}`);
58+
});
5459
return writeTail;
5560
};
5661

@@ -181,6 +186,9 @@ async function dispatch(message: JsonRpcMessage, ctx: DispatchCtx): Promise<void
181186
track(pipeEventStream(response, emitPayload, signal));
182187
return;
183188
}
189+
// The initialize response body is drained concurrently via track(); the
190+
// protocol version captured inside emitPayload→emit is set before the next
191+
// request fires because the MCP client awaits the initialize reply.
184192
track(forwardJsonBody(response, message, emit, emitPayload));
185193
}
186194

packages/cli-core/src/lib/input-json.test.ts

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import { test, expect, describe, beforeEach, afterEach } from "bun:test";
2-
import { expandInputJson, toKebabCase } from "./input-json.ts";
2+
import { expandInputJson, toKebabCase, GLOBAL_FLAGS_WITH_VALUE } from "./input-json.ts";
33
import { join } from "node:path";
44
import { mkdtemp, rm } from "node:fs/promises";
55
import { tmpdir } from "node:os";
@@ -393,3 +393,18 @@ describe("expandInputJson", () => {
393393
});
394394
});
395395
});
396+
397+
describe("GLOBAL_FLAGS_WITH_VALUE", () => {
398+
test("covers every root value-option in cli-program.ts", async () => {
399+
// If a new value-flag is added to the root program but omitted from
400+
// GLOBAL_FLAGS_WITH_VALUE, its value can leak into positionals and
401+
// incorrectly trigger the `mcp run` stdin bypass.
402+
const { createProgram } = await import("../cli-program.ts");
403+
const program = createProgram();
404+
const missing = program.options
405+
.filter((opt) => (opt.required || opt.optional) && opt.long)
406+
.map((opt) => opt.long!)
407+
.filter((flag) => !GLOBAL_FLAGS_WITH_VALUE.has(flag));
408+
expect(missing).toEqual([]);
409+
});
410+
});

packages/cli-core/src/lib/input-json.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -141,7 +141,8 @@ function hasStdinPipe(): boolean {
141141
// sync with the root options in cli-program.ts: if a value-flag isn't listed,
142142
// its value (e.g. `--mode mcp`) leaks into the positionals and could misfire
143143
// the `mcp run` detection below.
144-
const GLOBAL_FLAGS_WITH_VALUE = new Set(["--mode", "--input-json"]);
144+
// Exported so tests can verify this set covers all root value-options.
145+
export const GLOBAL_FLAGS_WITH_VALUE = new Set(["--mode", "--input-json"]);
145146

146147
function ownsRawStdin(argv: string[]): boolean {
147148
// Drop flags (and the values of known global value-flags) so what remains is

0 commit comments

Comments
 (0)