Merge pull request #1640 from DeusData/fix/json-mcp-extra-keys

fix(cli): accept an MCP entry the client annotated, instead of refusing it

src/cli/cli.c

@@ -1979,7 +1979,8 @@ static int cbm_json_mcp_snapshot_ownership(const char *document, size_t document
     char *command = NULL;
     int result = cbm_json_like_match_object_entry(document, document_length, object_path, path_len,
                                                   entry_name, fields, field_count, &command);
-    if (result == CBM_JSON_LIKE_OBJECT_MATCH &&
+    if ((result == CBM_JSON_LIKE_OBJECT_MATCH ||
+         result == CBM_JSON_LIKE_OBJECT_MATCH_WITH_EXTRAS) &&
         !cbm_json_mcp_owned_command(command, expected_binary, previous_managed_binary)) {
 #ifdef _WIN32
         result = cbm_json_mcp_command_availability(command) == CBM_JSON_MCP_COMMAND_MISSING
@@ -2010,6 +2011,26 @@ static int cbm_upsert_json_named_mcp(const char *binary_path, const char *config
         int ownership = cbm_json_mcp_snapshot_ownership(
             document, document_length, object_path, path_len, schema, entry_name, argument,
             binary_path, g_previous_managed_mcp_command);
+        /* An entry that already says what we would say, but carries extra keys
+         * the client added, is ALREADY SATISFIED. Return success without
+         * touching the file.
+         *
+         * We must not rewrite it: the editor replaces an entry wholesale, so
+         * writing our canonical shape over it would delete those keys. Doing
+         * nothing is both correct and lossless — the entry already points at
+         * this binary with the right type, which is the whole content of the
+         * install.
+         *
+         * This is #1630: OpenCode writes `"enabled": true` next to our
+         * `command` and `type`, so every user who had toggled a server in the
+         * UI hit `op=mcp_install` failure. Confirmed on Linux and Windows with
+         * two independent configs. Merging our fields into an annotated entry
+         * while preserving the rest is the fuller fix and is tracked there;
+         * this makes the common case work without risking anyone's config. */
+        if (ownership == CBM_JSON_LIKE_OBJECT_MATCH_WITH_EXTRAS) {
+            free(document);
+            return CLI_OK;
+        }
         /* STALE (our exact shape, dead binary path) is repairable — that is
          * the update contract. Only a genuinely foreign shape refuses. */
         if (ownership != CBM_JSON_LIKE_OBJECT_MATCH && ownership != CBM_JSON_LIKE_OBJECT_MISSING &&

src/cli/config_json_like.c

@@ -2834,10 +2834,28 @@ int cbm_json_like_match_object_entry(const char *document, size_t document_lengt
             free(decoded);
         }
     }
-    if (member_count != found_count || !captured) {
+    if (!captured) {
         free(captured);
         return CBM_JSON_LIKE_OBJECT_MISMATCH;
     }
+    if (member_count != found_count) {
+        /* Extra keys beyond the ones we own. Every field we DO own matched, so
+         * this entry is recognisably ours - it has just been annotated.
+         *
+         * OpenCode is the case that forced this: it writes `"enabled": true`
+         * alongside our `command` and `type`, and toggling a server on or off
+         * in the UI adds that key. Requiring an exact key set therefore made us
+         * classify our OWN entry as foreign and refuse to touch it, so install
+         * failed for anyone who had ever toggled a server (#1630, confirmed on
+         * Linux and Windows with two independent configs where every MCP server
+         * carried the key).
+         *
+         * Reported distinctly from MATCH because the two demand different
+         * handling: a caller may not rewrite this entry, since the editor
+         * replaces an entry wholesale and would drop the extra keys. */
+        *captured_string_out = captured;
+        return CBM_JSON_LIKE_OBJECT_MATCH_WITH_EXTRAS;
+    }
     *captured_string_out = captured;
     return CBM_JSON_LIKE_OBJECT_MATCH;
 }

src/cli/config_json_like.h

@@ -65,6 +65,12 @@ enum {
     CBM_JSON_LIKE_OBJECT_MATCH = 0,
     CBM_JSON_LIKE_OBJECT_MISSING = 1,
     CBM_JSON_LIKE_OBJECT_MISMATCH = 2,
+    /* Every field we own is present and matches, but the entry carries
+     * ADDITIONAL keys we do not write. The entry is recognisably ours; it has
+     * simply been annotated by the client or the user. Callers must NOT rewrite
+     * such an entry — the editor replaces an entry wholesale, so rewriting
+     * would silently drop those keys. Treat it as already-satisfied instead. */
+    CBM_JSON_LIKE_OBJECT_MATCH_WITH_EXTRAS = 3,
 };
 
 /* captured_string_out receives malloc-owned decoded content only on MATCH.

tests/test_cli.c

@@ -7415,6 +7415,88 @@ TEST(cli_opencode_prefers_existing_jsonc_config_discussion1560) {
     PASS();
 }
 
+/* #1630: OpenCode writes `"enabled": true` beside our `command` and `type`, so
+ * an entry we wrote ourselves carries three keys. Our ownership check demanded
+ * an exact key set, classified our own entry as foreign, and refused the whole
+ * install. Confirmed on Linux (#1630) and Windows (#1582) with two independent
+ * configs in which EVERY MCP server carried the key.
+ *
+ * The entry below is the reporters' actual shape. It already says what we would
+ * say, so the correct outcome is success WITHOUT a write - rewriting it would
+ * drop `enabled`, turning a visible refusal into silent config loss. */
+TEST(cli_opencode_accepts_entry_annotated_with_enabled_issue1630) {
+    char tmpdir[256];
+    snprintf(tmpdir, sizeof(tmpdir), "/tmp/cli-opencode-enabled-XXXXXX");
+    if (!cbm_mkdtemp(tmpdir))
+        FAIL("cbm_mkdtemp failed");
+    char config_path[512];
+    snprintf(config_path, sizeof(config_path), "%s/opencode.json", tmpdir);
+    const char *original = "{\n"
+                           "  \"$schema\": \"https://opencode.ai/config.json\",\n"
+                           "  \"mcp\": {\n"
+                           "    \"codebase-memory-mcp\": {\n"
+                           "      \"enabled\": true,\n"
+                           "      \"type\": \"local\",\n"
+                           "      \"command\": [\n"
+                           "        \"/usr/local/bin/codebase-memory-mcp\"\n"
+                           "      ]\n"
+                           "    }\n"
+                           "  }\n"
+                           "}\n";
+    write_test_file(config_path, original);
+
+    int rc = cbm_upsert_opencode_mcp("/usr/local/bin/codebase-memory-mcp", config_path);
+
+    char *after = read_test_file_alloc(config_path);
+    bool preserved = after && strstr(after, "\"enabled\": true") != NULL;
+    bool unchanged = after && strcmp(after, original) == 0;
+    free(after);
+    test_rmdir_r(tmpdir);
+    if (rc != 0)
+        FAIL("an entry annotated with enabled must be accepted, not refused");
+    if (!preserved)
+        FAIL("the client's enabled key must survive");
+    if (!unchanged)
+        FAIL("an already-correct entry must not be rewritten at all");
+    PASS();
+}
+
+/* The other direction: an entry whose command points at a DIFFERENT binary is
+ * genuinely foreign and must still be refused, extra keys or not. Without this
+ * the change above would be a blanket loosening. */
+TEST(cli_opencode_still_refuses_foreign_command_issue1630) {
+    char tmpdir[256];
+    snprintf(tmpdir, sizeof(tmpdir), "/tmp/cli-opencode-foreign-XXXXXX");
+    if (!cbm_mkdtemp(tmpdir))
+        FAIL("cbm_mkdtemp failed");
+    char config_path[512];
+    snprintf(config_path, sizeof(config_path), "%s/opencode.json", tmpdir);
+    const char *original = "{\n"
+                           "  \"mcp\": {\n"
+                           "    \"codebase-memory-mcp\": {\n"
+                           "      \"enabled\": true,\n"
+                           "      \"type\": \"local\",\n"
+                           "      \"command\": [\n"
+                           "        \"/opt/somebody-elses/binary\"\n"
+                           "      ]\n"
+                           "    }\n"
+                           "  }\n"
+                           "}\n";
+    write_test_file(config_path, original);
+
+    int rc = cbm_upsert_opencode_mcp("/usr/local/bin/codebase-memory-mcp", config_path);
+
+    char *after = read_test_file_alloc(config_path);
+    bool unchanged = after && strcmp(after, original) == 0;
+    free(after);
+    test_rmdir_r(tmpdir);
+    if (rc == 0)
+        FAIL("an entry pointing at a foreign binary must still be refused");
+    if (!unchanged)
+        FAIL("a refused entry must be left byte-identical");
+    PASS();
+}
+
 TEST(cli_opencode_config_dir_detects_without_retargeting_global_json) {
     char tmpdir[256];
     snprintf(tmpdir, sizeof(tmpdir), "/tmp/cli-opencode-dir-XXXXXX");
@@ -12669,6 +12751,8 @@ SUITE(cli) {
     RUN_TEST(cli_antigravity_plan_uses_documented_global_files);
     RUN_TEST(cli_opencode_honors_custom_config);
     RUN_TEST(cli_opencode_prefers_existing_jsonc_config_discussion1560);
+    RUN_TEST(cli_opencode_accepts_entry_annotated_with_enabled_issue1630);
+    RUN_TEST(cli_opencode_still_refuses_foreign_command_issue1630);
     RUN_TEST(cli_opencode_config_dir_detects_without_retargeting_global_json);
     RUN_TEST(cli_kiro_and_hermes_homes_are_honored);
     RUN_TEST(cli_detect_agents_finds_official_kiro_cli_executable);