fix(mcp): echo unmodeled state on full-replace connector/skill writes

Both full-replace write paths now preserve stored state the form doesn't model,
matching the skill metadata-edit path (and the admin fix):

- Connector edit: toPluginUpsert seeds the modeled server from the stored raw
  server (keeps cwd/disabled/timeout/autoApprove/…), re-emits other mcpServers
  entries, and re-emits non-modeled attachments verbatim. updateMcpReal pulls
  these from the freshly-fetched current record. A metadata edit (e.g. slogan)
  no longer destroys a Cline `disabled:true`, a second server, or a sixth file.
- Skill re-upload: EditSkillModal (no visibility control) now threads the skill's
  current visibility, and importBody fails closed to `private` when absent — a
  re-upload can no longer let a backend default widen a private skill.

Adds regression tests for both (connector unmodeled-field/second-server/extra-
attachment survival; skill re-upload fail-closed + explicit-visibility).

Author: lsc

packages/dmworkmcp/src/api/mcpService.connector.test.ts

@@ -243,3 +243,74 @@ describe("updateMcpReal — icon write intent uses an undefined sentinel (P1-1 /
     expect(upsertIcon()).toBe("icons/fresh-upload.png");
   });
 });
+
+describe("updateMcpReal — full-replace echoes unmodeled connector state", () => {
+  // A stored connector whose mcp.json carries fields the form doesn't model
+  // (cwd/disabled/timeout), a SECOND server, and a non-modeled attachment.
+  function richDetail() {
+    return detailPlugin({
+      plugin_json: {
+        $schema: "cowork-plugin-package-2.0.json",
+        connector: { type: "mcp", source: "connector.github-mcp" },
+        attachments: [
+          {
+            path: "mcp.json",
+            content_type: "raw",
+            raw_content: JSON.stringify({
+              mcpServers: {
+                "github-mcp": {
+                  type: "streamable-http",
+                  url: "https://mcp.example.com/github",
+                  cwd: "/srv/app",
+                  disabled: true,
+                  timeout: 60,
+                },
+                "other-server": { command: "node", args: ["x.js"] },
+              },
+            }),
+          },
+          { path: "connector/tools.json", content_type: "raw", raw_content: "[]" },
+          { path: "connector/custom.json", content_type: "raw", raw_content: '{"kept":true}' },
+        ],
+      },
+    });
+  }
+
+  function writtenUpsert() {
+    const call = mock.instance.post.mock.calls.find((c) =>
+      (c[0] as string).endsWith("/plugins/upsert")
+    ) as [string, { plugin: { plugin_json: { attachments: { path: string; raw_content: string }[] } } }];
+    const atts = call[1].plugin.plugin_json.attachments;
+    const mcp = atts.find((a) => a.path === "mcp.json")!;
+    return {
+      servers: (JSON.parse(mcp.raw_content) as { mcpServers: Record<string, Record<string, unknown>> })
+        .mcpServers,
+      paths: atts.map((a) => a.path),
+    };
+  }
+
+  it("preserves unmodeled server fields, a second server, and a non-modeled attachment on a metadata edit", async () => {
+    mock.instance.get.mockImplementation((url: string) => {
+      if (url.includes("/plugin_categories")) return Promise.resolve(categoriesOk());
+      if (url.includes("/plugins/detail")) return Promise.resolve({ data: { data: richDetail() } });
+      throw new Error(`unexpected GET ${url}`);
+    });
+    mock.instance.post.mockResolvedValue({ data: { data: detailPlugin() } });
+
+    // A metadata edit (new slogan); everything else the form re-derives.
+    await updateMcp("p-1", baseParams({ slogan: "Renamed" }));
+
+    const { servers, paths } = writtenUpsert();
+    // Unmodeled keys on the modeled server survive.
+    expect(servers["github-mcp"].cwd).toBe("/srv/app");
+    expect(servers["github-mcp"].disabled).toBe(true);
+    expect(servers["github-mcp"].timeout).toBe(60);
+    // Modeled fields still written from the form.
+    expect(servers["github-mcp"].url).toBe("https://mcp.example.com/github");
+    // The second server is not collapsed.
+    expect(servers["other-server"]).toEqual({ command: "node", args: ["x.js"] });
+    // The non-modeled attachment survives alongside the rebuilt five.
+    expect(paths).toContain("connector/custom.json");
+    expect(paths).toContain("mcp.json");
+  });
+});

packages/dmworkmcp/src/api/mcpService.ts

@@ -843,6 +843,22 @@ async function updateMcpReal(
   // sides come from the same fetch), so the service does no display comparison.
   const canonicalIcon =
     params.icon === undefined ? current.plugin.icon ?? "" : params.icon;
+  // The upsert replaces plugin_json wholesale and the form only models six
+  // server fields + five attachments. Extract the rest from the freshly-fetched
+  // current record so the write echoes it back instead of destroying it: the raw
+  // modeled-server object (keeps cwd/disabled/timeout/autoApprove/…), any other
+  // mcpServers entry, and any non-modeled attachment.
+  const currentServers =
+    jsonAttachment<McpJSONWire>(current.plugin.plugin_json, "mcp.json")
+      ?.mcpServers ?? {};
+  const currentServerName = Object.keys(currentServers)[0] ?? "";
+  const rawServer = currentServers[currentServerName] as
+    | Record<string, unknown>
+    | undefined;
+  const extraServers: Record<string, unknown> = {};
+  for (const [k, v] of Object.entries(currentServers)) {
+    if (k !== currentServerName) extraServers[k] = v;
+  }
   const detail = await post<PluginDetailWire>(
     "/plugins/upsert",
     toPluginUpsert(
@@ -851,6 +867,10 @@ async function updateMcpReal(
         pluginId: id,
         categoryId,
         visibility: current.plugin.visibility,
+        rawServer,
+        extraServers,
+        // toPluginUpsert drops the five modeled paths, keeping only the extras.
+        extraAttachments: current.plugin.plugin_json?.attachments,
       }
     )
   );

packages/dmworkmcp/src/api/mcpWireParams.ts

@@ -3,6 +3,7 @@ import { slugifyServerName } from "../utils/constants";
 import {
   SECRET_PLACEHOLDER,
   goCanonicalJSON,
+  type PluginAttachmentWire,
   type PluginManifestWire,
   type PluginVisibilityWire,
 } from "./pluginWire";
@@ -19,6 +20,23 @@ interface PluginAttachmentBody {
   raw_content: string;
 }
 
+/** The connector-package attachments this form fully models and rebuilds from
+ *  the current form on every write. Any OTHER stored attachment is preserved
+ *  verbatim (opts.extraAttachments): the upsert replaces plugin_json wholesale,
+ *  so a path we neither model nor re-emit would be silently dropped on edit. */
+const MODELED_ATTACHMENT_PATHS = new Set([
+  "mcp.json",
+  "connector/tools.json",
+  "connector/examples.json",
+  "connector/faqs.json",
+  "connector/notes.json",
+]);
+
+/** Modeled server-object keys the form owns; everything else on the stored
+ *  server (cwd, disabled, timeout, autoApprove, …) is seeded back from
+ *  opts.rawServer so a metadata edit doesn't destroy it. */
+const MODELED_SERVER_KEYS = ["type", "url", "command", "args", "env", "headers"];
+
 export interface PluginUpsertBody {
   plugin: {
     plugin_id?: string;
@@ -32,7 +50,7 @@ export interface PluginUpsertBody {
     plugin_json: {
       $schema: string;
       connector: { type: "mcp"; source: string };
-      attachments: PluginAttachmentBody[];
+      attachments: (PluginAttachmentBody | PluginAttachmentWire)[];
     };
   };
   relations: [];
@@ -42,6 +60,14 @@ export interface PluginUpsertOptions {
   pluginId?: string;
   categoryId?: string;
   visibility: PluginVisibilityWire;
+  /** The RAW stored modeled-server object, seeded into the write so keys this
+   *  form doesn't model (cwd/disabled/timeout/autoApprove/…) survive an edit. */
+  rawServer?: Record<string, unknown>;
+  /** Other mcpServers entries, re-emitted verbatim so a multi-server document
+   *  isn't collapsed to one on a metadata edit. */
+  extraServers?: Record<string, unknown>;
+  /** Stored attachments outside MODELED_ATTACHMENT_PATHS, re-emitted verbatim. */
+  extraAttachments?: PluginAttachmentWire[];
 }
 
 export function toPluginUpsert(
@@ -76,7 +102,12 @@ export function toPluginUpsert(
   // client therefore never SENDS secret values — user-supplied env/header keys
   // are emitted as ${KEY} placeholders (filled locally at install time), and a
   // redaction sentinel echoed from a read is blanked before write.
-  const server: Record<string, unknown> = {};
+  // Seed from the RAW stored modeled-server object so keys this form does not
+  // model (cwd, disabled, timeout, autoApprove, …) survive a metadata edit; then
+  // drop the modeled keys and overlay the form, so clearing a field (deleting
+  // env/headers, clearing args) doesn't leave a stale seeded value behind.
+  const server: Record<string, unknown> = { ...(opts.rawServer ?? {}) };
+  for (const k of MODELED_SERVER_KEYS) delete server[k];
   if (params.transport) server.type = params.transport;
   if (params.url) server.url = params.url;
   if (params.command) server.command = params.command;
@@ -96,8 +127,15 @@ export function toPluginUpsert(
   // mapDetail reads serverName back from this key, so a display-name key would
   // regress the detail snippet too.
   const serverKey = slug || name;
-  const attachments: PluginAttachmentBody[] = [
-    rawAtt("mcp.json", goCanonicalJSON({ mcpServers: { [serverKey]: server } })),
+  // Re-emit any other stored servers verbatim (minus the one we're writing) so a
+  // multi-server document isn't collapsed on a metadata edit.
+  const mcpServers: Record<string, unknown> = {};
+  for (const [k, v] of Object.entries(opts.extraServers ?? {})) {
+    if (k !== serverKey) mcpServers[k] = v;
+  }
+  mcpServers[serverKey] = server;
+  const attachments: (PluginAttachmentBody | PluginAttachmentWire)[] = [
+    rawAtt("mcp.json", goCanonicalJSON({ mcpServers })),
     rawAtt("connector/tools.json", goCanonicalJSON(params.tools ?? [])),
     rawAtt("connector/examples.json", goCanonicalJSON(usage)),
     rawAtt(
@@ -109,6 +147,11 @@ export function toPluginUpsert(
       goCanonicalJSON((params.notes ?? []).map((s) => s.trim()).filter(Boolean))
     ),
   ];
+  // Preserve any stored attachment this form doesn't model (guard against a
+  // stale extra colliding with a modeled path — the modeled rebuild wins).
+  for (const att of opts.extraAttachments ?? []) {
+    if (!MODELED_ATTACHMENT_PATHS.has(att.path)) attachments.push(att);
+  }
   return {
     plugin: {
       ...(opts.pluginId ? { plugin_id: opts.pluginId } : {}),

packages/dmworkskillmarket/src/api/skillApiReal.test.ts

@@ -512,6 +512,28 @@ describe("skillApiReal", () => {
     expect(body.visibility).toBe("private");
   });
 
+  it("re-upload fails CLOSED to private when the caller omits visibility (full replace)", async () => {
+    mockFetch.mockReturnValueOnce(
+      jsonResponse({ plugin: pluginSkillWire(), relations: [] })
+    );
+    // EditSkillModal has no visibility control; if it ever omits the field, the
+    // full-replace import must not let a backend default widen a private skill.
+    await updateSkill("new-skill", { parseTaskId: "task-3" });
+    const [, init] = mockFetch.mock.calls[mockFetch.mock.calls.length - 1] as [string, RequestInit];
+    const body = JSON.parse(init.body as string);
+    expect(body.visibility).toBe("private");
+  });
+
+  it("re-upload preserves an explicit visibility the modal threads through", async () => {
+    mockFetch.mockReturnValueOnce(
+      jsonResponse({ plugin: pluginSkillWire(), relations: [] })
+    );
+    await updateSkill("new-skill", { parseTaskId: "task-4", visibility: "space" });
+    const [, init] = mockFetch.mock.calls[mockFetch.mock.calls.length - 1] as [string, RequestInit];
+    const body = JSON.parse(init.body as string);
+    expect(body.visibility).toBe("space");
+  });
+
   it("updateSkill without a parse task merges onto the current documents and upserts", async () => {
     const current = pluginSkillWire({
       plugin_json: {

packages/dmworkskillmarket/src/api/skillApiReal.ts

@@ -543,7 +543,10 @@ function importBody(
     description: form.description,
     category_id: form.categoryId || undefined,
     tags: form.tags,
-    visibility: form.visibility,
+    // A re-upload is a full replace: never send an absent visibility (JSON drops
+    // `undefined`), which would let a backend default decide whether a private
+    // skill stays private. Fail closed to the most restrictive value.
+    visibility: form.visibility ?? "private",
     version: form.version,
     changelog: form.changelog,
   };

packages/dmworkskillmarket/src/components/EditSkillModal.tsx

@@ -391,6 +391,10 @@ export default function EditSkillModal({ skill, categories, onClose, onUpdated }
         description,
         categoryId,
         tags: submittedTags,
+        // This modal has no visibility control, so preserve the skill's current
+        // visibility explicitly — a re-upload is a full replace and would
+        // otherwise send no visibility, leaving it to a backend default.
+        visibility: skill.visibility,
         ...(iconUrl !== undefined ? { iconUrl } : {}),
       });
       onUpdated(updated);