Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 22 additions & 1 deletion src/cli/cli.c
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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 &&
Expand Down
20 changes: 19 additions & 1 deletion src/cli/config_json_like.c
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
6 changes: 6 additions & 0 deletions src/cli/config_json_like.h
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
84 changes: 84 additions & 0 deletions tests/test_cli.c
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand Down Expand Up @@ -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);
Expand Down
Loading