From 6d27c115d8d819ea8d3a50ec938cb925dcffd93c Mon Sep 17 00:00:00 2001 From: nntrivi2001 Date: Wed, 9 Sep 2026 09:36:52 +0700 Subject: [PATCH] =?UTF-8?q?fix(cc-config):=20MCP=20scope=20was=20inverted?= =?UTF-8?q?=20=E2=80=94=20expose=20user/local,=20not=20user/project?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit readMcpServers only ever parses ~/.claude.json (top-level = user scope, projects[] = the panel's "projectScoped" bucket, which the CLI itself calls `local`). The previous user/project scope choice was the exact inverse: `--scope project` writes .mcp.json, which readMcpServers never reads (a successful add would vanish), and every "projectScoped" row was actually `local`, so Remove sent a scope the CLI could never match. - cc-mcp-cli.js: MCP_SCOPES -> ["user", "local"]; drop the local mutationQueue/serialize in favor of cc-plugin-cli.js's exported one, so a plugin install and an MCP add can't spawn `claude` concurrently; drop the String(v) coercion in env/header flags (a wrong-typed value is now a 400, not a written "[object Object]"); assertEmbeddedValue allows empty strings (a legitimate empty stdio arg was previously a 400). - cc-plugin-cli.js: export `serialize` for reuse. - CcConfig.tsx: CcMcpScope narrowed to user/local; the projectScoped bucket's McpCard now sends scope="local"; rows sourced from ~/.claude/settings.json (no CLI scope addresses them) no longer render a Remove button. Cleaned up i18n keys my own earlier change made dead (explain.mcp.addCmd/remove/removeButton) and gave the scope
{(["stdio", "http"] as const).map((k) => ( @@ -1994,7 +1994,7 @@ function McpPanel({
@@ -2063,29 +2067,33 @@ function McpCard({ )}
- { - if (!confirmRemove) { - setConfirmRemove(true); - return; - } - setConfirmRemove(false); - void run( - busyId, - () => api.ccConfig.mcpRemove(server.name, scope), - t("manage.mcpRemoved", { name: server.name }) - ); - }} - > - {busy === busyId - ? t("manage.working") - : confirmRemove - ? t("manage.confirmRemove") - : t("manage.remove")} - + {removable ? ( + { + if (!confirmRemove) { + setConfirmRemove(true); + return; + } + setConfirmRemove(false); + void run( + busyId, + () => api.ccConfig.mcpRemove(server.name, scope), + t("manage.mcpRemoved", { name: server.name }) + ); + }} + > + {busy === busyId + ? t("manage.working") + : confirmRemove + ? t("manage.confirmRemove") + : t("manage.remove")} + + ) : ( +

{t("mcp.notRemovable")}

+ )}
); diff --git a/docs/API.md b/docs/API.md index bf36b53a..65debfb2 100644 --- a/docs/API.md +++ b/docs/API.md @@ -1607,7 +1607,7 @@ POST /api/cc-config/mcp/remove Body: { name, scope? } **Plugins and marketplaces.** These five endpoints shell out to the `claude plugin …` CLI with an explicit argv array (never a shell string), pinned to the dashboard's `CLAUDE_HOME` via `CLAUDE_CONFIG_DIR`; mutating calls are serialized in-process. `source` must be `owner/repo`, an `http(s)://` URL, or an absolute path; `spec`/`key` must be `` or `@`; `scope` is `user` (default), `project` or `local`. Validation failures return 400 `EBADARG`; a missing `claude` binary returns 503 `ECLINOTFOUND`; a CLI timeout returns 504 `ETIMEOUT`; a non-zero CLI exit returns **422 `ECLIEXIT`** with the CLI's message plus `stdout`/`stderr`. Successful runs return `{ ok: true, stdout, stderr }` and broadcast `cc_config_changed`. `GET /plugins/available` proxies `claude plugin list --available --json` as `{ installed, available }`. -**MCP servers.** `POST /api/cc-config/mcp/add` and `POST /api/cc-config/mcp/remove` shell out to `claude mcp add`/`claude mcp remove` the same way the plugin routes shell out to `claude plugin …` — an argv array, never a shell string, serialized in-process. `scope` is `user` or `project` only (required on add, optional on remove — omitted, the CLI removes from whichever scope the server exists in); the settings union's third value `project-local` is a different axis and is not accepted here. `add` takes either `url` (+ optional `transport: "http"|"sse"`, `headers`) for a remote server, or `command` + `args` (+ optional `env`) for a stdio server — exactly one of the two forms. Validation failures return 400 `EBADARG`; a non-zero CLI exit returns 422 `ECLIEXIT`. `GET /api/cc-config/mcp` is unaffected — it still reads `~/.claude.json` directly; the client refetches it after either call succeeds. +**MCP servers.** `POST /api/cc-config/mcp/add` and `POST /api/cc-config/mcp/remove` shell out to `claude mcp add`/`claude mcp remove` the same way the plugin routes shell out to `claude plugin …` — an argv array, never a shell string, serialized on the SAME in-process queue `cc-plugin-cli.js` exports (`serialize`), so a plugin install and an MCP add can never spawn `claude` concurrently. `scope` is `user` or `local` only (required on add, optional on remove — omitted, the CLI removes from whichever scope the server exists in). This is the inverse of what it sounds like: `user` writes `~/.claude.json`'s top level, `local` writes its `projects[]` block — the exact source `GET /mcp`'s `projectScoped` bucket reads. The CLI's own `project` scope (a separate, shared `.mcp.json`) is refused with 400: `readMcpServers` never reads that file, so a server added there would report success and never appear in the list. `add` takes either `url` (+ optional `transport: "http"|"sse"`, `headers`) for a remote server, or `command` + `args` (+ optional `env`) for a stdio server — exactly one of the two forms. Validation failures return 400 `EBADARG`; a non-zero CLI exit returns 422 `ECLIEXIT`. `GET /api/cc-config/mcp` is unaffected — it still reads `~/.claude.json` directly; the client refetches it after either call succeeds. Rows sourced from `~/.claude/settings.json` have no CLI scope that addresses them, so the panel does not offer Remove on them. `PUT /api/cc-config/keybindings` edits `~/.claude/keybindings.json` from a structured list of context groups (`{ groups: [{ context, bindings: [{ key, action }] }] }`). The server backs the file up first (under `/cc-config-backups/keybindings/`), preserves any top-level metadata (`$schema`/`$docs`), and replaces only the `bindings` array; duplicate contexts or duplicate keys within a context return `EBADCONTENT`. `keybindings.json` is replaced wholesale here because the live CLI does not rewrite it mid-session; `settings.json`, which it does, is only ever key-merged (see `PATCH /settings` above). diff --git a/openapi.yaml b/openapi.yaml index 89d41bdf..d81eb39f 100644 --- a/openapi.yaml +++ b/openapi.yaml @@ -8016,7 +8016,7 @@ paths: tags: - CcConfig summary: Add an MCP server - description: 'Mutating. Runs `claude mcp add` — never a direct edit of ~/.claude.json, which the running CLI rewrites concurrently. Body carries either `url` (+ optional `transport: http|sse` and `headers`) for a remote server, or `command` + `args` (+ optional `env`) for a stdio server; exactly one of the two forms is required. `scope` is required and is `user`|`project` only — not the settings union''s third `project-local` value, which is a different axis. A non-zero CLI exit is returned as 422, not 500. Emits a `cc_config_changed` websocket event on success.' + description: 'Mutating. Runs `claude mcp add` — never a direct edit of ~/.claude.json, which the running CLI rewrites concurrently. Body carries either `url` (+ optional `transport: http|sse` and `headers`) for a remote server, or `command` + `args` (+ optional `env`) for a stdio server; exactly one of the two forms is required. `scope` is required and is `user`|`local` only: `user` writes ~/.claude.json''s top level, `local` writes its `projects[]` block (GET /mcp''s `projectScoped` bucket). The CLI''s own `project` scope (a shared `.mcp.json`) is deliberately not offered — `readMcpServers` never reads that file, so a server added there would report success and never appear in the list. A non-zero CLI exit is returned as 422, not 500. Emits a `cc_config_changed` websocket event on success.' operationId: ccConfigPostMcpAdd requestBody: required: true @@ -8035,7 +8035,7 @@ paths: type: string enum: - user - - project + - local example: user transport: type: string @@ -8083,7 +8083,7 @@ paths: tags: - CcConfig summary: Remove an MCP server - description: Mutating. Runs `claude mcp remove`. `scope` (`user`|`project`) narrows which config layer to remove from; omit it to let the CLI find the server wherever it exists. A non-zero CLI exit (e.g. the name does not exist) is returned as 422. Emits a `cc_config_changed` websocket event on success. + description: Mutating. Runs `claude mcp remove`. `scope` (`user`|`local`) narrows which config layer to remove from; omit it to let the CLI find the server wherever it exists. The CLI's own `project` scope is refused with 400 — the panel has no row sourced from `.mcp.json` to remove. A non-zero CLI exit (e.g. the name does not exist) is returned as 422. Emits a `cc_config_changed` websocket event on success. operationId: ccConfigPostMcpRemove requestBody: required: true @@ -8101,7 +8101,7 @@ paths: type: string enum: - user - - project + - local responses: '200': description: The CLI's stdout/stderr on success. diff --git a/server/__tests__/cc-config.test.js b/server/__tests__/cc-config.test.js index ca9064a7..5b58eeac 100644 --- a/server/__tests__/cc-config.test.js +++ b/server/__tests__/cc-config.test.js @@ -1192,7 +1192,7 @@ describe("/api/cc-config", () => { try { const { status, body } = await fetchJson("/api/cc-config/mcp/remove", { method: "POST", - body: { name: "docs", scope: "project" }, + body: { name: "docs", scope: "local" }, }); assert.equal(status, 200); assert.deepEqual(body.stdout.trim().split("\n"), [ @@ -1200,10 +1200,19 @@ describe("/api/cc-config", () => { "remove", "docs", "--scope", - "project", + "local", ]); } finally { restore(); } }); + + it("POST /mcp/remove rejects the CLI's project scope with 400 — .mcp.json isn't addressable here", async () => { + const { status, body } = await fetchJson("/api/cc-config/mcp/remove", { + method: "POST", + body: { name: "docs", scope: "project" }, + }); + assert.equal(status, 400); + assert.equal(body.error.code, "EBADARG"); + }); }); diff --git a/server/__tests__/cc-mcp-cli.test.js b/server/__tests__/cc-mcp-cli.test.js index 19af82d0..6839d4f1 100644 --- a/server/__tests__/cc-mcp-cli.test.js +++ b/server/__tests__/cc-mcp-cli.test.js @@ -65,11 +65,18 @@ describe("cc-mcp-cli argument validation", () => { assert.equal(cli.assertMcpScope("user", { required: true }), "user"); }); - it("rejects the settings-only project-local scope and any other unknown scope", () => { - for (const bad of ["project-local", "local", "bogus"]) { + it("rejects the CLI's own `project` scope — readMcpServers never reads .mcp.json", () => { + // The Critical this pins: `--scope project` writes .mcp.json, which the + // dashboard's read path never parses, so a successful add would report + // an "added" toast for a server that never appears in the list. + for (const bad of ["project", "project-local", "bogus"]) { assert.throws(() => cli.assertMcpScope(bad), /scope must be one of/, `accepted ${bad}`); } }); + + it("accepts local — the CLI scope that actually backs the panel's projectScoped bucket", () => { + assert.equal(cli.assertMcpScope("local", { required: true }), "local"); + }); }); describe("cc-mcp-cli argv construction", () => { @@ -119,7 +126,7 @@ describe("cc-mcp-cli argv construction", () => { try { const result = await cli.mcpAdd({ name: "corridor", - scope: "project", + scope: "local", transport: "http", url: "https://app.corridor.dev/api/mcp", headers: { Authorization: "Bearer xyz" }, @@ -128,7 +135,7 @@ describe("cc-mcp-cli argv construction", () => { "mcp", "add", "--scope", - "project", + "local", "--transport", "http", "corridor", @@ -172,18 +179,45 @@ describe("cc-mcp-cli argv construction", () => { const restore = stubClaude(); try { assert.deepEqual(argvOf(await cli.mcpRemove({ name: "docs" })), ["mcp", "remove", "docs"]); - assert.deepEqual(argvOf(await cli.mcpRemove({ name: "docs", scope: "project" })), [ + assert.deepEqual(argvOf(await cli.mcpRemove({ name: "docs", scope: "user" })), [ "mcp", "remove", "docs", "--scope", - "project", + "user", ]); } finally { restore(); } }); + it("builds a local-scoped remove — the argv the CLI actually accepts for the panel's projectScoped rows", async () => { + // The other Critical this pins: the panel used to send scope: "project" + // for these rows, which the real CLI never matches (it stores them under + // "local"), so Remove failed every time. `argvOf` is the exact argv + // `execFile` spawns — there is no shell in between to paper over a wrong + // scope string. + const restore = stubClaude(); + try { + assert.deepEqual(argvOf(await cli.mcpRemove({ name: "docs", scope: "local" })), [ + "mcp", + "remove", + "docs", + "--scope", + "local", + ]); + } finally { + restore(); + } + }); + + it("rejects a remove scoped to the CLI's project (.mcp.json is not addressable here)", async () => { + await assert.rejects( + () => cli.mcpRemove({ name: "docs", scope: "project" }), + /scope must be one of/ + ); + }); + it("rejects url and command together, and neither", async () => { await assert.rejects( () => cli.mcpAdd({ name: "x", scope: "user", url: "https://a", command: "b" }), @@ -212,6 +246,48 @@ describe("cc-mcp-cli argv construction", () => { ); }); + it("allows an empty stdio arg — only type/length/control-char bounds are enforced", async () => { + // A trailing empty argv element is invisible to `.trim().split("\n")` + // (it collapses with the final newline), so assert with a marker arg + // after it rather than checking the last line directly. + const restore = stubClaude(); + try { + const result = await cli.mcpAdd({ + name: "x", + scope: "user", + command: "node", + args: ["", "marker"], + }); + assert.deepEqual(argvOf(result).slice(-3), ["node", "", "marker"]); + } finally { + restore(); + } + }); + + it("rejects an object env/header value as a 400 instead of writing [object Object]", async () => { + await assert.rejects( + () => + cli.mcpAdd({ + name: "x", + scope: "user", + command: "node", + env: { KEY: { nope: true } }, + }), + /KEY is required/ + ); + await assert.rejects( + () => + cli.mcpAdd({ + name: "x", + scope: "user", + transport: "http", + url: "https://a", + headers: { Authorization: { nope: true } }, + }), + /Authorization is required/ + ); + }); + it("never lets an args element reach a shell", async () => { // A shell-metacharacter payload is a legal argv element (many real flags // contain punctuation) — the guarantee is execFile's argv array, not a diff --git a/server/lib/cc-mcp-cli.js b/server/lib/cc-mcp-cli.js index 79484d39525fa09731dfe4fdc005107048dbe9a6..b089d1539920ceee45461f29c3428eeaf7f28720 100644 GIT binary patch delta 1093 zcmaJ=PiqrF6en82rll%Xs-@sFJ+zhlQ3ORRf}}l&)&}B1(Be*JU$WCoX4aY6Y~#W7 zrf1iCsaG#T@azZhTlh(Qv)dF=a3Kj~=l$OA-+X_1xbto6{8(zvWB``y7(rOTgIHS( z;n(437%;~pp?Z);%t2s~g*!ZMqZy*HK^bm>e>hXXxJ#InBd~}MAV@5lz@r848+9&O z5@LPdY88OfaZTb7WfcOydQ7{8okIP6e2oxdqxX?J`|6?5+CAC}phJUwSa%UZ%oNJC z)A`ED;kH*3R-lvcKDv!Myr8qqXHWjXJCQtw)Ch;Pr#|3uL9#lflSYC^|L6uq{0(?F zY;Ke8cF}m7go9KkG6WXVhrIAcO~E`}qCi%0jGYChLU2(9A!Ih{XlBCP$pYL)I;ALt zbr?>{voA}t4MG!4igy_+S4!3FLwRkhnuEMeFhTH~M&(8W28mY=tQ^+@+svMD$2dhb4aR7v} z&M)$Es0cgFmIw0&_+Ff-i5X1?MG*qvTJgjfRE`S6dQ2=426=&C*#Q0PuINE1Om=nd zdb#R-%#}~>&fO`EbNW;>zv@l9x!c-qPwDBwfIy+E4K;RjTAiY)dAGTwDte+}QNO%> zq@nnKGbbUGuQ!737QLUAszSOGwG~8`<&2)$C~D%YR6>E`OaV4JR|#K9x$t>}a_;aVw8!e*+Q-bwL0C delta 924 zcmZuwO>fgc5T&IO#6@jW2{-^^5QiklNe^6rs=WkMA!q@?fddC`y<>aZ&2HFT=Yt}p zKLKG5ocIR_B#Zxp=-=SLtet`?;gh}l=FOY;#>cxqo_v3JaW*vi0Am+{jr9;gSis3p zTjcQT_!B7Yz-0*ir;l5f-h|;bFn~MYpLQg5%w*IFVVsFL^8^gIND?%t9Iy!;Bt{P^ z8qRFCubJUQX3;sJc2e6ruv(q+MRPsfnieFRq5-BjjG3+EFndVi5^X>$uZZZi8I&iD z*9*m?9vx@@3%E)f(Ww_(-7gX2se;!q2(9Sn{H zj!D2+NLfu)m3pp3P%TTDP}cPVVzv&Bz~rhgS!rI3_j>TEHWHY$ z8ZsJZ?p+8RrPxQq9Li}mq^|tqkfA9?E4MGqT`9k;j24&9b#KnfR`c8W;?3o~VtXwp zK3wS*@7F$8H$|%%6(`MR@pFB(T)dhrHC{R+RN5YON^$Mu!oSbr_x1j41D+|0U3IBN zOmg9EjyYET;t0(Y2DxJtR0qv@zNb0D*a(Lv^nBzK0}?h6)+a+|pTftPCjV(k4t;m)w3+q- ziEb`;-VV0ld0pKzXtf++yR#VtNv>iiXnicsj9^l{!*-%Na1?;w5lWJBq}nsmd!$kk wEChmX67tL~p{b_E&>j649oX82TyacsDuXUJ$}^PrZ|pSw0BKx3O8@`> diff --git a/server/lib/cc-plugin-cli.js b/server/lib/cc-plugin-cli.js index f237da86..e462fe77 100644 --- a/server/lib/cc-plugin-cli.js +++ b/server/lib/cc-plugin-cli.js @@ -259,6 +259,10 @@ function cliMessage(res) { module.exports = { runClaude, cliMessage, + // Exported so cc-mcp-cli.js queues onto this SAME chokepoint — one CLI + // process against CLAUDE_CONFIG_DIR at a time, whether the click was a + // plugin install or an MCP add. + serialize, listAvailable, marketplaceAdd, marketplaceRemove, diff --git a/server/openapi-extra/cc-config.js b/server/openapi-extra/cc-config.js index 7f516f6e..b66a86f8 100644 --- a/server/openapi-extra/cc-config.js +++ b/server/openapi-extra/cc-config.js @@ -1597,7 +1597,7 @@ const paths = { tags: ["CcConfig"], summary: "Add an MCP server", description: - "Mutating. Runs `claude mcp add` — never a direct edit of ~/.claude.json, which the running CLI rewrites concurrently. Body carries either `url` (+ optional `transport: http|sse` and `headers`) for a remote server, or `command` + `args` (+ optional `env`) for a stdio server; exactly one of the two forms is required. `scope` is required and is `user`|`project` only — not the settings union's third `project-local` value, which is a different axis. A non-zero CLI exit is returned as 422, not 500. Emits a `cc_config_changed` websocket event on success.", + "Mutating. Runs `claude mcp add` — never a direct edit of ~/.claude.json, which the running CLI rewrites concurrently. Body carries either `url` (+ optional `transport: http|sse` and `headers`) for a remote server, or `command` + `args` (+ optional `env`) for a stdio server; exactly one of the two forms is required. `scope` is required and is `user`|`local` only: `user` writes ~/.claude.json's top level, `local` writes its `projects[]` block (GET /mcp's `projectScoped` bucket). The CLI's own `project` scope (a shared `.mcp.json`) is deliberately not offered — `readMcpServers` never reads that file, so a server added there would report success and never appear in the list. A non-zero CLI exit is returned as 422, not 500. Emits a `cc_config_changed` websocket event on success.", operationId: "ccConfigPostMcpAdd", requestBody: { required: true, @@ -1608,7 +1608,7 @@ const paths = { required: ["name", "scope"], properties: { name: { type: "string", example: "vercel" }, - scope: { type: "string", enum: ["user", "project"], example: "user" }, + scope: { type: "string", enum: ["user", "local"], example: "user" }, transport: { type: "string", enum: ["http", "sse"] }, url: { type: "string", example: "https://mcp.vercel.com" }, headers: { @@ -1652,7 +1652,7 @@ const paths = { tags: ["CcConfig"], summary: "Remove an MCP server", description: - "Mutating. Runs `claude mcp remove`. `scope` (`user`|`project`) narrows which config layer to remove from; omit it to let the CLI find the server wherever it exists. A non-zero CLI exit (e.g. the name does not exist) is returned as 422. Emits a `cc_config_changed` websocket event on success.", + "Mutating. Runs `claude mcp remove`. `scope` (`user`|`local`) narrows which config layer to remove from; omit it to let the CLI find the server wherever it exists. The CLI's own `project` scope is refused with 400 — the panel has no row sourced from `.mcp.json` to remove. A non-zero CLI exit (e.g. the name does not exist) is returned as 422. Emits a `cc_config_changed` websocket event on success.", operationId: "ccConfigPostMcpRemove", requestBody: { required: true, @@ -1663,7 +1663,7 @@ const paths = { required: ["name"], properties: { name: { type: "string", example: "vercel" }, - scope: { type: "string", enum: ["user", "project"] }, + scope: { type: "string", enum: ["user", "local"] }, }, }, },