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

OpenCode writes "enabled": true beside the "command" and "type" we write. Our
ownership check required the entry's key set to match EXACTLY
(config_json_like.c: member_count != found_count), so a three-key entry with two
recognised keys was classified as FOREIGN - and we refused to touch an entry we
had written ourselves. install then failed with:

  error: agent_config agent=OpenCode op=mcp_install path=.../opencode.json

Confirmed on two independent configs: Linux (#1630) and Windows (#1582). In
gotspatel's file EVERY MCP server carries the key - mssql, forgetful,
chrome-devtools and ours - so this is OpenCode's normal shape, not an unusual
hand-edit. Anyone who has ever toggled a server on or off in the UI was hit.

Two of my own hypotheses were wrong before the reporters' files settled it: it is
not JSONC comment parsing, and it is not the .jsonc targeting that #1575 fixed.
#1575 fixed WHICH file we open; this happens after, on what we find inside.

The distinction now reported is MATCH_WITH_EXTRAS, and the caller treats it as
ALREADY SATISFIED - success, without touching the file. That is deliberate and it
is the safe half of the fix: cbm_json_like_upsert_entry REPLACES an entry
wholesale, so writing our canonical shape over an annotated entry would silently
delete the client's keys. A refusal the user can see beats a deletion they
cannot. Doing nothing is also correct on the merits: the entry already names this
binary with the right type, which is the entire content of the install.

Merging our fields into an annotated entry while preserving the rest is the
fuller fix and stays tracked in #1630. This makes the common case work without
risking anyone's configuration tonight.

Ownership is NOT loosened otherwise: an entry whose command points at a different
binary is still foreign and still refused, byte-identically, and that direction
is pinned by its own test.

Tests use the reporters' actual entry shape. cli suite: 272 passed.

Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
This commit is contained in:
Martin Vogel
2026-08-14 19:19:02 +02:00
parent b9cde7e025
commit dbd20eaa48
4 changed files with 131 additions and 2 deletions
+22 -1
View File
@@ -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 &&
+19 -1
View File
@@ -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;
}
+6
View File
@@ -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.
+84
View File
@@ -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);