From 3a4160b4610f83ea26daf69f1ffe7cb898bf48ad Mon Sep 17 00:00:00 2001 From: Martin Vogel Date: Tue, 8 Sep 2026 20:38:22 +0200 Subject: [PATCH] fix(cli): complete the OpenHands integration (settings.json mcp_config + agent-profile refs) (#1826) install wrote only part of the OpenHands config. Register the server globally under ~/.openhands/settings.json -> mcp_config {transport:stdio, command, enabled} (owned-entry, unrelated content preserved), and add "codebase-memory-mcp" to each ~/.openhands/agent-profiles/

.json mcp_server_refs array (null -> [it], existing -> appended once). uninstall removes exactly what it added. Co-Authored-By: Claude Fable 5.1 Signed-off-by: Martin Vogel --- src/cli/cli.c | 209 ++++++++++++++++++++++++++++ src/cli/config_json_like.c | 66 ++++++++- src/cli/config_json_like.h | 5 + tests/test_cli.c | 248 ++++++++++++++++++++++++++++++++++ tests/test_config_json_like.c | 173 ++++++++++++++++++++++++ 5 files changed, 696 insertions(+), 5 deletions(-) diff --git a/src/cli/cli.c b/src/cli/cli.c index d85ebf0b6..21bb36d65 100644 --- a/src/cli/cli.c +++ b/src/cli/cli.c @@ -2282,6 +2282,141 @@ static int cbm_remove_openclaw_compaction(const char *config_path) { : CLI_ERR; } +/* ── OpenHands settings.json mcp_config (#1826) ─────────────── + * The mcpServers-style config installed above (cbm_install_editor_mcp into + * ~/.openhands/mcp.json) is not enough: OpenHands only loads a global MCP + * server it finds registered under settings.json -> mcp_config, in its own + * shape ({transport, command, enabled} — no args array). Agent profiles then + * opt in individually via mcp_server_refs (below). */ + +static size_t cbm_openhands_ownership_fields(cbm_json_like_object_field_t fields[3]) { + fields[0] = (cbm_json_like_object_field_t){ + .key = "transport", + .shape = CBM_JSON_LIKE_VALUE_STRING, + .expected_string = "stdio", + .flags = CBM_JSON_LIKE_FIELD_REQUIRED, + }; + fields[1] = (cbm_json_like_object_field_t){ + .key = "command", + .shape = CBM_JSON_LIKE_VALUE_STRING, + .expected_string = NULL, + .flags = CBM_JSON_LIKE_FIELD_REQUIRED | CBM_JSON_LIKE_FIELD_CAPTURE_STRING, + }; + fields[2] = (cbm_json_like_object_field_t){ + .key = "enabled", + .shape = CBM_JSON_LIKE_VALUE_LITERAL, + .expected_string = "true", + .flags = CBM_JSON_LIKE_FIELD_REQUIRED, + }; + return 3U; +} + +static char *cbm_build_openhands_mcp_entry(const char *binary_path) { + yyjson_mut_doc *doc = yyjson_mut_doc_new(NULL); + if (!doc) { + return NULL; + } + yyjson_mut_val *root = yyjson_mut_obj(doc); + bool ok = root && yyjson_mut_obj_add_strcpy(doc, root, "transport", "stdio") && + yyjson_mut_obj_add_strcpy(doc, root, "command", binary_path) && + yyjson_mut_obj_add_bool(doc, root, "enabled", true); + char *json = NULL; + if (ok) { + yyjson_mut_doc_set_root(doc, root); + json = yyjson_mut_write(doc, YYJSON_WRITE_NOFLAG, NULL); + } + yyjson_mut_doc_free(doc); + return json; +} + +/* Insert or leave alone: an already-owned entry (exact match, or annotated + * with extra keys the client added) needs no write. A same-named entry that + * does not match our shape is left untouched and reported as an error rather + * than overwritten, matching cbm_upsert_json_named_mcp's fail-closed rule for + * every other editor client. */ +static int cbm_upsert_openhands_settings_mcp(const char *binary_path, const char *settings_path) { + if (!binary_path || !settings_path) { + return CLI_ERR; + } + static const char *const path[] = {"mcp_config"}; + char *document = NULL; + size_t document_length = 0U; + int read_result = cbm_json_like_read_document(settings_path, &document, &document_length); + if (read_result < 0) { + return CLI_ERR; + } + if (read_result == 0) { + cbm_json_like_object_field_t fields[3]; + size_t field_count = cbm_openhands_ownership_fields(fields); + char *command = NULL; + int ownership = + cbm_json_like_match_object_entry(document, document_length, path, 1U, + CBM_DEFAULT_MCP_SERVER_NAME, fields, field_count, + &command); + free(command); + if (ownership == CBM_JSON_LIKE_OBJECT_MATCH || + ownership == CBM_JSON_LIKE_OBJECT_MATCH_WITH_EXTRAS) { + free(document); + return CLI_OK; + } + if (ownership != CBM_JSON_LIKE_OBJECT_MISSING) { + free(document); + return CLI_ERR; + } + } + char *entry = cbm_build_openhands_mcp_entry(binary_path); + if (!entry) { + free(document); + return CLI_ERR; + } + int edit_result = cbm_json_like_upsert_entry_if_unchanged( + settings_path, path, 1U, CBM_DEFAULT_MCP_SERVER_NAME, entry, + read_result == 1 ? NULL : document, document_length); + free(entry); + free(document); + return edit_result == 0 ? CLI_OK : CLI_ERR; +} + +/* Remove only an entry that is still recognisably ours (an annotated entry is + * left in place and reported, same rule as insertion above); a missing file, + * path, or entry is a successful no-op. */ +static int cbm_remove_openhands_settings_mcp(const char *settings_path) { + if (!settings_path) { + return CLI_ERR; + } + static const char *const path[] = {"mcp_config"}; + char *document = NULL; + size_t document_length = 0U; + int read_result = cbm_json_like_read_document(settings_path, &document, &document_length); + if (read_result == 1) { + free(document); + return CLI_OK; + } + if (read_result < 0) { + free(document); + return CLI_ERR; + } + cbm_json_like_object_field_t fields[3]; + size_t field_count = cbm_openhands_ownership_fields(fields); + char *command = NULL; + int ownership = cbm_json_like_match_object_entry(document, document_length, path, 1U, + CBM_DEFAULT_MCP_SERVER_NAME, fields, + field_count, &command); + free(command); + if (ownership == CBM_JSON_LIKE_OBJECT_MISSING || ownership == CBM_JSON_LIKE_OBJECT_MISMATCH) { + free(document); + return CLI_OK; + } + if (ownership != CBM_JSON_LIKE_OBJECT_MATCH) { + free(document); + return CLI_ERR; + } + int edit_result = cbm_json_like_remove_entry_if_unchanged( + settings_path, path, 1U, CBM_DEFAULT_MCP_SERVER_NAME, document, document_length); + free(document); + return edit_result == 0 ? CLI_OK : CLI_ERR; +} + /* ── VS Code MCP (servers key with type:stdio) ────────────────── */ int cbm_install_vscode_mcp(const char *binary_path, const char *config_path) { @@ -9228,6 +9363,49 @@ static void uninstall_vscode_profile_configs(const char *code_user, const char * cbm_closedir(directory); } +static bool cbm_filename_has_suffix(const char *name, const char *suffix) { + size_t name_len = strlen(name); + size_t suffix_len = strlen(suffix); + return name_len >= suffix_len && strcmp(name + (name_len - suffix_len), suffix) == 0; +} + +/* Register or unregister our server against every existing OpenHands agent + * profile's mcp_server_refs array (#1826). A missing agent-profiles/ + * directory is a silent no-op in both directions — install must never invent + * it, and uninstall has nothing to undo there. Only *.json entries are + * touched; a profile directory may hold arbitrary notes alongside profiles. */ +static void openhands_update_profile_refs(const char *profiles_dir, bool installing, + bool dry_run) { + cbm_dir_t *d = cbm_opendir(profiles_dir); + if (!d) { + return; + } + cbm_dirent_t *ent; + while ((ent = cbm_readdir(d)) != NULL) { + if (strcmp(ent->name, ".") == 0 || strcmp(ent->name, "..") == 0 || + !cbm_filename_has_suffix(ent->name, ".json")) { + continue; + } + char profile_path[CLI_BUF_1K]; + snprintf(profile_path, sizeof(profile_path), "%s/%s", profiles_dir, ent->name); + struct stat state; + if (stat(profile_path, &state) != 0 || !S_ISREG(state.st_mode) || dry_run) { + continue; + } + int result = installing ? cbm_json_like_add_unique_string( + profile_path, "mcp_server_refs", CBM_DEFAULT_MCP_SERVER_NAME) + : cbm_json_like_remove_string(profile_path, "mcp_server_refs", + CBM_DEFAULT_MCP_SERVER_NAME); + if (result != CLI_OK) { + record_agent_config_error(!installing, "OpenHands", + installing ? "profile_refs_install" + : "profile_refs_uninstall", + profile_path); + } + } + cbm_closedir(d); +} + /* Install MCP configs for editor-based agents (Zed, KiloCode, VS Code, OpenClaw). */ static void install_editor_agent_configs(const cbm_detected_agents_t *agents, const char *home, const char *binary_path, bool force, bool dry_run) { @@ -9490,11 +9668,32 @@ static void install_additional_agent_configs(const cbm_detected_agents_t *agents if (agents->openhands) { char cp[CLI_BUF_1K]; char skills_dir[CLI_BUF_1K]; + char settings_path[CLI_BUF_1K]; + char profiles_dir[CLI_BUF_1K]; snprintf(cp, sizeof(cp), "%s/.openhands/mcp.json", home); snprintf(skills_dir, sizeof(skills_dir), "%s/.agents/skills", home); + snprintf(settings_path, sizeof(settings_path), "%s/.openhands/settings.json", home); + snprintf(profiles_dir, sizeof(profiles_dir), "%s/.openhands/agent-profiles", home); install_generic_agent_config("OpenHands", binary_path, cp, NULL, dry_run, cbm_install_editor_mcp); install_agent_skill("OpenHands", skills_dir, force, dry_run); + /* #1826: the mcpServers-shaped mcp.json above is not enough — OpenHands + * only loads a server registered under settings.json -> mcp_config, and + * only for agent profiles that reference it. agent-profiles/ is never + * invented; a missing directory means nothing to register into yet. */ + if (g_install_plan) { + plan_record("OpenHands", "mcp_config", settings_path); + } else { + if (!dry_run) { + if (!prepare_config_parent(settings_path) || + cbm_upsert_openhands_settings_mcp(binary_path, settings_path) != CLI_OK) { + record_agent_config_error(false, "OpenHands", "settings_mcp_install", + settings_path); + } + } + printf(" settings mcp_config: %s\n", settings_path); + openhands_update_profile_refs(profiles_dir, true, dry_run); + } } if (agents->augment) { char cp[CLI_BUF_1K]; @@ -11750,11 +11949,21 @@ static void uninstall_additional_agents(const cbm_detected_agents_t *agents, con if (agents->openhands) { char cp[CLI_BUF_1K]; char skills_dir[CLI_BUF_1K]; + char settings_path[CLI_BUF_1K]; + char profiles_dir[CLI_BUF_1K]; snprintf(cp, sizeof(cp), "%s/.openhands/mcp.json", home); snprintf(skills_dir, sizeof(skills_dir), "%s/.agents/skills", home); + snprintf(settings_path, sizeof(settings_path), "%s/.openhands/settings.json", home); + snprintf(profiles_dir, sizeof(profiles_dir), "%s/.openhands/agent-profiles", home); uninstall_agent_mcp_instr((mcp_uninstall_args_t){"OpenHands", cp, NULL}, dry_run, cbm_remove_editor_mcp_owned); printf(" removed %d skill(s)\n", cbm_remove_skills(skills_dir, dry_run)); + /* #1826 counterpart: undo the settings.json registration and every + * agent profile's mcp_server_refs entry the install above added. */ + if (!dry_run && cbm_remove_openhands_settings_mcp(settings_path) != CLI_OK) { + record_agent_config_error(true, "OpenHands", "settings_mcp_uninstall", settings_path); + } + openhands_update_profile_refs(profiles_dir, false, dry_run); } if (agents->augment) { char cp[CLI_BUF_1K]; diff --git a/src/cli/config_json_like.c b/src/cli/config_json_like.c index dc16c3c5c..3ac7802a3 100644 --- a/src/cli/config_json_like.c +++ b/src/cli/config_json_like.c @@ -1223,8 +1223,19 @@ static int jl_make_insertion(const char *text, size_t length, size_t object_star return 0; } - if (object->close_pos == gap_start || - !jl_is_space((unsigned char)text[object->close_pos - 1U])) { + /* Zero-width insertion right at close_pos: whatever byte already sits at + * close_pos - 1 is untouched and stays in the output. When the original + * had a real gap there (close_pos != gap_start, e.g. "[ \"a\" ]"), that + * preserved byte already supplies a separator on the leading side, so + * mirroring it on the trailing side (space before the bracket) matches + * the array's own loose style. The fully tight case ("[\"a\"]", nothing + * at all between the last value and the bracket) has no such byte to + * lean on: manufacture ONE space so `,new` reads `, new` — the + * comma-spacing convention every other insertion path uses — and add + * nothing after, so `new]` stays `new]` rather than gaining a trailing + * space the original never had (#1826 byte-for-byte round-trip). */ + bool tight = object->close_pos == gap_start; + if (tight || !jl_is_space((unsigned char)text[object->close_pos - 1U])) { if (jl_buffer_char(insertion, ' ') != 0) { return -1; } @@ -1235,7 +1246,7 @@ static int jl_make_insertion(const char *text, size_t length, size_t object_star if (object->trailing_comma && jl_buffer_char(insertion, ',') != 0) { return -1; } - return jl_buffer_char(insertion, ' '); + return tight ? 0 : jl_buffer_char(insertion, ' '); } static int jl_apply_edits(const char *source, size_t source_length, jl_edit_t *edits, @@ -2406,6 +2417,13 @@ int cbm_json_like_remove_entry_if_unchanged(const char *file_path, const char *c expected_length); } +/* True when [start, end) is the bare 4-byte literal token null (never a + * quoted "null" string, which is 6 bytes with the quotes) — the documented + * OpenHands "no list yet" shape for mcp_server_refs (#1826). */ +static bool jl_is_null_literal(const char *text, size_t start, size_t end) { + return end - start == 4U && memcmp(text + start, "null", 4U) == 0; +} + int cbm_json_like_add_unique_string_at_path(const char *file_path, const char *const *object_path, size_t path_len, const char *array_key, const char *string_value) { @@ -2497,6 +2515,24 @@ int cbm_json_like_add_unique_string_at_path(const char *file_path, const char *c result = jl_insert_member(source, source_length, object_start, &object, object_path, path_len, SIZE_MAX, array_key, array_json.data, array_json.length, &updated, &updated_length); + } else if (jl_is_null_literal(source, object.match.value_start, object.match.value_end)) { + /* OpenHands profiles ship `"mcp_server_refs": null` as the documented + * "no list yet" shape (#1826). Splice the built one-element array over + * the bare null token in place, preserving every other byte (trailing + * comments included) the way the object-member and array-element + * edits below already do for their own value spans. */ + size_t head = object.match.value_start; + size_t tail = object.match.value_end; + updated_length = head + array_json.length + (source_length - tail); + updated = (char *)malloc(updated_length + 1U); + if (!updated) { + result = -1; + } else { + memcpy(updated, source, head); + memcpy(updated + head, array_json.data, array_json.length); + memcpy(updated + head + array_json.length, source + tail, source_length - tail); + updated[updated_length] = '\0'; + } } else if (source[object.match.value_start] != '[') { result = -1; } else { @@ -2759,6 +2795,24 @@ static int jl_decode_field_string(const char *text, size_t start, size_t end, } return jl_decode_string_value(text, start, end, value_out) == 0 ? 0 : 1; } + if (shape == CBM_JSON_LIKE_VALUE_LITERAL) { + /* The value's token boundaries already exclude surrounding trivia + * (jl_parse_value advances pos past exactly the token). Copy the raw + * bytes verbatim so a quoted "true" never equals the bare literal + * true, and the caller's expected_string comparison decides match. */ + if (start >= end) { + return 1; + } + size_t length = end - start; + char *copy = (char *)malloc(length + 1U); + if (!copy) { + return 1; + } + memcpy(copy, text + start, length); + copy[length] = '\0'; + *value_out = copy; + return 0; + } if (shape != CBM_JSON_LIKE_VALUE_SINGLE_STRING_ARRAY || start >= end || text[start] != '[') { return 1; } @@ -2827,11 +2881,13 @@ int cbm_json_like_match_object_entry(const char *document, size_t document_lengt size_t capture_count = 0U; for (size_t i = 0U; i < field_count; ++i) { if (!fields[i].key || fields[i].key[0] == '\0' || - fields[i].shape > CBM_JSON_LIKE_VALUE_SINGLE_STRING_ARRAY || + fields[i].shape > CBM_JSON_LIKE_VALUE_LITERAL || (fields[i].flags & ~(CBM_JSON_LIKE_FIELD_REQUIRED | CBM_JSON_LIKE_FIELD_CAPTURE_STRING)) != 0U || ((fields[i].flags & CBM_JSON_LIKE_FIELD_CAPTURE_STRING) != 0U && - fields[i].shape == CBM_JSON_LIKE_VALUE_EMPTY_ARRAY)) { + (fields[i].shape == CBM_JSON_LIKE_VALUE_EMPTY_ARRAY || + fields[i].shape == CBM_JSON_LIKE_VALUE_LITERAL)) || + (fields[i].shape == CBM_JSON_LIKE_VALUE_LITERAL && !fields[i].expected_string)) { return -1; } capture_count += (fields[i].flags & CBM_JSON_LIKE_FIELD_CAPTURE_STRING) != 0U ? 1U : 0U; diff --git a/src/cli/config_json_like.h b/src/cli/config_json_like.h index 23a8687e5..57cbfbfbf 100644 --- a/src/cli/config_json_like.h +++ b/src/cli/config_json_like.h @@ -47,6 +47,11 @@ typedef enum { CBM_JSON_LIKE_VALUE_STRING, CBM_JSON_LIKE_VALUE_EMPTY_ARRAY, CBM_JSON_LIKE_VALUE_SINGLE_STRING_ARRAY, + /* An exact bare token (true/false/null/a number) matched byte-for-byte + * against expected_string. Requires expected_string and may not be + * combined with CBM_JSON_LIKE_FIELD_CAPTURE_STRING — a fixed token has + * nothing meaningful to capture for the caller. */ + CBM_JSON_LIKE_VALUE_LITERAL, } cbm_json_like_value_shape_t; enum { diff --git a/tests/test_cli.c b/tests/test_cli.c index 976280868..a8d793644 100644 --- a/tests/test_cli.c +++ b/tests/test_cli.c @@ -3814,6 +3814,251 @@ TEST(cli_openclaw_uninstall_removes_compaction_when_workspace_is_ambiguous) { PASS(); } +/* ═══════════════════════════════════════════════════════════════════ + * OpenHands settings.json + agent-profiles registration (#1826) + * + * A bare ~/.openhands/mcp.json is not enough: OpenHands registers global MCP + * servers under settings.json → mcp_config, and every agent profile under + * agent-profiles/ must list the server in mcp_server_refs before that agent + * may use it. Foreign content survives byte-for-byte; uninstall removes + * exactly what install added. + * ═══════════════════════════════════════════════════════════════════ */ + +typedef struct { + char home[256]; + char binary[512]; /* the path `uninstall` expects: /.local/bin/... */ + char settings[640]; + char mcp[640]; + char profiles_dir[640]; + char default_profile[768]; + char custom_profile[768]; + char note[768]; + char *saved_home; + char *saved_path; + char *saved_cache; +} openhands_fixture_t; + +static bool openhands_fixture_open(openhands_fixture_t *fx, bool with_profiles) { + snprintf(fx->home, sizeof(fx->home), "/tmp/cli-openhands-XXXXXX"); + if (!cbm_mkdtemp(fx->home)) { + return false; + } + snprintf(fx->settings, sizeof(fx->settings), "%s/.openhands/settings.json", fx->home); +#ifdef _WIN32 + snprintf(fx->binary, sizeof(fx->binary), "%s/.local/bin/codebase-memory-mcp.exe", fx->home); +#else + snprintf(fx->binary, sizeof(fx->binary), "%s/.local/bin/codebase-memory-mcp", fx->home); +#endif + snprintf(fx->mcp, sizeof(fx->mcp), "%s/.openhands/mcp.json", fx->home); + snprintf(fx->profiles_dir, sizeof(fx->profiles_dir), "%s/.openhands/agent-profiles", fx->home); + snprintf(fx->default_profile, sizeof(fx->default_profile), "%s/default.json", fx->profiles_dir); + snprintf(fx->custom_profile, sizeof(fx->custom_profile), "%s/custom.json", fx->profiles_dir); + snprintf(fx->note, sizeof(fx->note), "%s/README.txt", fx->profiles_dir); + char openhands_dir[512]; + snprintf(openhands_dir, sizeof(openhands_dir), "%s/.openhands", fx->home); + test_mkdirp(with_profiles ? fx->profiles_dir : openhands_dir); + fx->saved_home = save_test_env("HOME"); + fx->saved_path = save_test_env("PATH"); + fx->saved_cache = save_test_env("CBM_CACHE_DIR"); + cbm_setenv("HOME", fx->home, 1); + cbm_setenv("PATH", fx->home, 1); + cbm_unsetenv("CBM_CACHE_DIR"); + return true; +} + +static void openhands_fixture_close(openhands_fixture_t *fx) { + restore_test_env("HOME", fx->saved_home); + restore_test_env("PATH", fx->saved_path); + restore_test_env("CBM_CACHE_DIR", fx->saved_cache); + test_rmdir_r(fx->home); +} + +static yyjson_doc *openhands_read_doc(const char *path) { + char *data = read_test_file_alloc(path); + if (!data) { + return NULL; + } + yyjson_doc *doc = yyjson_read(data, strlen(data), 0); + free(data); + return doc; +} + +/* settings.json → mcp_config. must be {transport:"stdio", command:, + * enabled:true}; returns false for any deviation. */ +static bool openhands_settings_entry_ok(const char *settings_path, const char *name, + const char *binary) { + yyjson_doc *doc = openhands_read_doc(settings_path); + yyjson_val *root = doc ? yyjson_doc_get_root(doc) : NULL; + yyjson_val *mcp_config = root ? yyjson_obj_get(root, "mcp_config") : NULL; + yyjson_val *entry = mcp_config ? yyjson_obj_get(mcp_config, name) : NULL; + bool ok = entry && yyjson_is_obj(entry) && yyjson_obj_size(entry) == 3U; + yyjson_val *transport = ok ? yyjson_obj_get(entry, "transport") : NULL; + yyjson_val *command = ok ? yyjson_obj_get(entry, "command") : NULL; + yyjson_val *enabled = ok ? yyjson_obj_get(entry, "enabled") : NULL; + ok = ok && transport && yyjson_is_str(transport) && + strcmp(yyjson_get_str(transport), "stdio") == 0; + ok = ok && command && yyjson_is_str(command) && strcmp(yyjson_get_str(command), binary) == 0; + ok = ok && enabled && yyjson_is_true(enabled); + yyjson_doc_free(doc); + return ok; +} + +static bool openhands_settings_entry_absent(const char *settings_path, const char *name) { + yyjson_doc *doc = openhands_read_doc(settings_path); + yyjson_val *root = doc ? yyjson_doc_get_root(doc) : NULL; + yyjson_val *mcp_config = root ? yyjson_obj_get(root, "mcp_config") : NULL; + bool absent = root && (!mcp_config || !yyjson_obj_get(mcp_config, name)); + yyjson_doc_free(doc); + return absent; +} + +/* profile → mcp_server_refs must be exactly the given strings in order. */ +static bool openhands_profile_refs_equal(const char *profile_path, const char *const *expected, + size_t expected_count) { + yyjson_doc *doc = openhands_read_doc(profile_path); + yyjson_val *root = doc ? yyjson_doc_get_root(doc) : NULL; + yyjson_val *refs = root ? yyjson_obj_get(root, "mcp_server_refs") : NULL; + bool ok = refs && yyjson_is_arr(refs) && yyjson_arr_size(refs) == expected_count; + for (size_t i = 0; ok && i < expected_count; i++) { + yyjson_val *item = yyjson_arr_get(refs, i); + ok = item && yyjson_is_str(item) && strcmp(yyjson_get_str(item), expected[i]) == 0; + } + yyjson_doc_free(doc); + return ok; +} + +static bool openhands_file_equals(const char *path, const char *expected) { + char *data = read_test_file_alloc(path); + bool equal = data && expected && strcmp(data, expected) == 0; + free(data); + return equal; +} + +static const char openhands_settings_before[] = "{\n" + " \"language\": \"en\",\n" + " \"mcp_config\": {\n" + " \"other-mcp\": {\"transport\": \"stdio\", " + "\"command\": \"/opt/other\", \"enabled\": false}\n" + " },\n" + " \"llm_model\": \"gpt-x\"\n" + "}\n"; +static const char openhands_default_profile_before[] = "{\n" + " \"name\": \"default\",\n" + " \"mcp_server_refs\": null,\n" + " \"tools\": [\"bash\"]\n" + "}\n"; +static const char openhands_custom_profile_before[] = + "{\"name\": \"custom\", \"mcp_server_refs\": [\"other-mcp\"]}\n"; +static const char openhands_note_before[] = "not a profile\n"; + +/* Foreign settings keys and a foreign mcp_config sibling survive verbatim; + * a null profile ref list becomes ours, an existing list is appended once; + * a second install is byte-idempotent; uninstall restores the foreign files + * byte-for-byte and removes only our ref. */ +TEST(cli_openhands_registers_settings_mcp_config_and_profile_refs_issue1826) { + openhands_fixture_t fx; + if (!openhands_fixture_open(&fx, true)) + FAIL("cbm_mkdtemp failed"); + write_test_file(fx.settings, openhands_settings_before); + write_test_file(fx.default_profile, openhands_default_profile_before); + write_test_file(fx.custom_profile, openhands_custom_profile_before); + write_test_file(fx.note, openhands_note_before); + + cbm_install_agent_configs(fx.home, fx.binary, false, false); + + const char *const only_ours[] = {"codebase-memory-mcp"}; + const char *const appended[] = {"other-mcp", "codebase-memory-mcp"}; + bool ours_ok = openhands_settings_entry_ok(fx.settings, "codebase-memory-mcp", fx.binary); + char *settings_after = read_test_file_alloc(fx.settings); + bool foreign_ok = + settings_after && strstr(settings_after, "\"language\": \"en\"") && + strstr(settings_after, "\"llm_model\": \"gpt-x\"") && + strstr(settings_after, "\"other-mcp\": {\"transport\": \"stdio\", \"command\": " + "\"/opt/other\", \"enabled\": false}"); + bool default_ok = openhands_profile_refs_equal(fx.default_profile, only_ours, 1U); + bool custom_ok = openhands_profile_refs_equal(fx.custom_profile, appended, 2U); + char *default_after = read_test_file_alloc(fx.default_profile); + bool default_foreign_ok = default_after && strstr(default_after, "\"name\": \"default\"") && + strstr(default_after, "\"tools\": [\"bash\"]"); + bool note_ok = openhands_file_equals(fx.note, openhands_note_before); + const char *const standard_json[] = {"mcpServers", "codebase-memory-mcp", fx.binary}; + bool mcp_json_ok = test_file_contains_all(fx.mcp, standard_json, 3); + char *custom_after = read_test_file_alloc(fx.custom_profile); + + /* Second install: byte-idempotent on every touched file. */ + cbm_install_agent_configs(fx.home, fx.binary, false, false); + bool idempotent = openhands_file_equals(fx.settings, settings_after) && + openhands_file_equals(fx.default_profile, default_after) && + openhands_file_equals(fx.custom_profile, custom_after); + free(settings_after); + free(default_after); + free(custom_after); + + char *argv[] = {"uninstall", "--yes"}; + int rc = cli_test_cmd_uninstall(2, argv); + bool settings_restored = openhands_file_equals(fx.settings, openhands_settings_before); + bool custom_restored = + openhands_file_equals(fx.custom_profile, openhands_custom_profile_before); + /* null → ["codebase-memory-mcp"] → [] : the ref is gone; an empty list is + * the minimal edit (the editor cannot know the list was null before). */ + bool default_cleared = openhands_profile_refs_equal(fx.default_profile, only_ours, 0U); + bool note_restored = openhands_file_equals(fx.note, openhands_note_before); + openhands_fixture_close(&fx); + + if (!ours_ok) + FAIL("settings.json must register mcp_config.codebase-memory-mcp {stdio, binary, enabled}"); + if (!foreign_ok) + FAIL("settings.json foreign keys and the foreign mcp_config sibling must survive verbatim"); + if (!default_ok) + FAIL("a null mcp_server_refs must become [\"codebase-memory-mcp\"]"); + if (!custom_ok) + FAIL("an existing mcp_server_refs list must gain codebase-memory-mcp exactly once"); + if (!default_foreign_ok) + FAIL("profile keys around mcp_server_refs must survive verbatim"); + if (!note_ok) + FAIL("non-JSON files under agent-profiles must not be touched"); + if (!mcp_json_ok) + FAIL("the existing ~/.openhands/mcp.json registration must be kept"); + if (!idempotent) + FAIL("a second install must be byte-idempotent"); + if (rc != 0 || !settings_restored) + FAIL("uninstall must restore settings.json byte-for-byte"); + if (!custom_restored) + FAIL("uninstall must restore a profile with foreign refs byte-for-byte"); + if (!default_cleared) + FAIL("uninstall must remove our ref from the null-origin profile"); + if (!note_restored) + FAIL("uninstall must not touch non-JSON files under agent-profiles"); + PASS(); +} + +/* Fresh ~/.openhands without settings.json or agent-profiles: settings.json is + * created with only our entry, no profile directory is invented, and uninstall + * removes the entry again. */ +TEST(cli_openhands_creates_settings_and_skips_missing_profiles_issue1826) { + openhands_fixture_t fx; + if (!openhands_fixture_open(&fx, false)) + FAIL("cbm_mkdtemp failed"); + + cbm_install_agent_configs(fx.home, fx.binary, false, false); + bool ours_ok = openhands_settings_entry_ok(fx.settings, "codebase-memory-mcp", fx.binary); + struct stat st; + bool no_profiles_invented = stat(fx.profiles_dir, &st) != 0; + + char *argv[] = {"uninstall", "--yes"}; + int rc = cli_test_cmd_uninstall(2, argv); + bool removed = openhands_settings_entry_absent(fx.settings, "codebase-memory-mcp"); + openhands_fixture_close(&fx); + + if (!ours_ok) + FAIL("a missing settings.json must be created with mcp_config.codebase-memory-mcp"); + if (!no_profiles_invented) + FAIL("install must never create ~/.openhands/agent-profiles"); + if (rc != 0 || !removed) + FAIL("uninstall must remove mcp_config.codebase-memory-mcp from settings.json"); + PASS(); +} + /* ═══════════════════════════════════════════════════════════════════ * VS Code MCP config tests * ═══════════════════════════════════════════════════════════════════ */ @@ -5168,6 +5413,7 @@ TEST(cli_new_agent_install_plans_use_documented_paths) { "/.hermes/skills/codebase-memory/SKILL.md", "\"openhands\"", "/.openhands/mcp.json", + "/.openhands/settings.json", "/.agents/skills/codebase-memory/SKILL.md", "\"cline\"", "/.cline/mcp.json", @@ -14858,6 +15104,8 @@ SUITE(cli) { RUN_TEST(cli_openclaw_mcp_preserves_valid_json5); RUN_TEST(cli_openclaw_mcp_uninstall_uses_nested_servers); RUN_TEST(cli_openclaw_compaction_preserves_user_owned_section); + RUN_TEST(cli_openhands_registers_settings_mcp_config_and_profile_refs_issue1826); + RUN_TEST(cli_openhands_creates_settings_and_skips_missing_profiles_issue1826); RUN_TEST(cli_openclaw_profile_uses_profile_state_and_default_workspace); RUN_TEST(cli_openclaw_uninstall_removes_compaction_when_workspace_is_ambiguous); diff --git a/tests/test_config_json_like.c b/tests/test_config_json_like.c index c6f29a387..8e6a2b28c 100644 --- a/tests/test_config_json_like.c +++ b/tests/test_config_json_like.c @@ -671,6 +671,176 @@ TEST(config_json_like_top_level_array_unique_string) { PASS(); } +/* OpenHands profiles ship `"mcp_server_refs": null` (#1826). A null value is + * the documented "no list yet" shape, so adding a unique string must replace + * exactly that token with a one-element array, preserving everything around + * it, and stay idempotent. Removal leaves the minimal edit: an empty array. */ +TEST(config_json_like_array_add_replaces_null_value_issue1826) { + jl_fixture_t fixture; + ASSERT_EQ(jl_fixture_open(&fixture), 0); + const char *original = "{\n" + " \"name\": \"default\",\n" + " \"mcp_server_refs\": null, // keep this user comment\n" + " \"tools\": [\"bash\"]\n" + "}\n"; + ASSERT_EQ(jl_write(fixture.path, original), 0); + ASSERT_EQ( + cbm_json_like_add_unique_string(fixture.path, "mcp_server_refs", "codebase-memory-mcp"), 0); + char *first = jl_read(fixture.path); + ASSERT_NOT_NULL(first); + ASSERT_STR_EQ(first, + "{\n" + " \"name\": \"default\",\n" + " \"mcp_server_refs\": [\"codebase-memory-mcp\"], // keep this user comment\n" + " \"tools\": [\"bash\"]\n" + "}\n"); + + ASSERT_EQ( + cbm_json_like_add_unique_string(fixture.path, "mcp_server_refs", "codebase-memory-mcp"), 0); + char *second = jl_read(fixture.path); + ASSERT_NOT_NULL(second); + ASSERT_STR_EQ(second, first); + free(first); + free(second); + + ASSERT_EQ(cbm_json_like_remove_string(fixture.path, "mcp_server_refs", "codebase-memory-mcp"), + 0); + char *removed = jl_read(fixture.path); + ASSERT_NOT_NULL(removed); + ASSERT_STR_EQ(removed, "{\n" + " \"name\": \"default\",\n" + " \"mcp_server_refs\": [], // keep this user comment\n" + " \"tools\": [\"bash\"]\n" + "}\n"); + free(removed); + + /* Only the literal null converts; any other non-array value still fails + * closed byte-identically. */ + const char *wrong_type = "{\"mcp_server_refs\": \"null\", \"keep\": true}\n"; + ASSERT_EQ(jl_write(fixture.path, wrong_type), 0); + ASSERT_EQ(cbm_json_like_add_unique_string(fixture.path, "mcp_server_refs", "x"), -1); + char *content = jl_read(fixture.path); + ASSERT_NOT_NULL(content); + ASSERT_STR_EQ(content, wrong_type); + free(content); + jl_fixture_close(&fixture); + PASS(); +} + +/* The OpenHands settings entry carries `enabled: true` (#1826). A literal shape + * lets ownership matching require that token exactly, so uninstall recognises + * our own entry instead of treating the boolean as a foreign annotation. */ +static int jl_match_openhands_entry(const char *document, + const cbm_json_like_object_field_t *fields, size_t field_count, + char **captured) { + static const char *const path[] = {"mcp_config"}; + return cbm_json_like_match_object_entry(document, strlen(document), path, 1U, + "codebase-memory-mcp", fields, field_count, captured); +} + +TEST(config_json_like_match_object_entry_literal_shape_issue1826) { + const cbm_json_like_object_field_t fields[] = { + {.key = "command", + .shape = CBM_JSON_LIKE_VALUE_STRING, + .expected_string = NULL, + .flags = CBM_JSON_LIKE_FIELD_REQUIRED | CBM_JSON_LIKE_FIELD_CAPTURE_STRING}, + {.key = "transport", + .shape = CBM_JSON_LIKE_VALUE_STRING, + .expected_string = "stdio", + .flags = CBM_JSON_LIKE_FIELD_REQUIRED}, + {.key = "enabled", + .shape = CBM_JSON_LIKE_VALUE_LITERAL, + .expected_string = "true", + .flags = CBM_JSON_LIKE_FIELD_REQUIRED}, + }; + char *captured = NULL; + const char *ours = "{\"mcp_config\": {\"codebase-memory-mcp\": {\"transport\": \"stdio\", " + "\"command\": \"/x/cbm\", \"enabled\": true}}}\n"; + ASSERT_EQ(jl_match_openhands_entry(ours, fields, 3U, &captured), CBM_JSON_LIKE_OBJECT_MATCH); + ASSERT_NOT_NULL(captured); + ASSERT_STR_EQ(captured, "/x/cbm"); + free(captured); + + /* Trivia around the token is not part of the value. */ + const char *spaced = "{\"mcp_config\": {\"codebase-memory-mcp\": {\"transport\": \"stdio\", " + "\"command\": \"/x/cbm\", \"enabled\" : true /* on */ }}}\n"; + ASSERT_EQ(jl_match_openhands_entry(spaced, fields, 3U, &captured), CBM_JSON_LIKE_OBJECT_MATCH); + free(captured); + + const char *disabled = "{\"mcp_config\": {\"codebase-memory-mcp\": {\"transport\": \"stdio\", " + "\"command\": \"/x/cbm\", \"enabled\": false}}}\n"; + ASSERT_EQ(jl_match_openhands_entry(disabled, fields, 3U, &captured), + CBM_JSON_LIKE_OBJECT_MISMATCH); + ASSERT_NULL(captured); + + const char *quoted = "{\"mcp_config\": {\"codebase-memory-mcp\": {\"transport\": \"stdio\", " + "\"command\": \"/x/cbm\", \"enabled\": \"true\"}}}\n"; + ASSERT_EQ(jl_match_openhands_entry(quoted, fields, 3U, &captured), + CBM_JSON_LIKE_OBJECT_MISMATCH); + ASSERT_NULL(captured); + + const char *missing = "{\"mcp_config\": {\"codebase-memory-mcp\": {\"transport\": \"stdio\", " + "\"command\": \"/x/cbm\"}}}\n"; + ASSERT_EQ(jl_match_openhands_entry(missing, fields, 3U, &captured), + CBM_JSON_LIKE_OBJECT_MISMATCH); + ASSERT_NULL(captured); + + /* A literal field can neither lack its token nor be captured. */ + const cbm_json_like_object_field_t no_token[] = { + fields[0], + {.key = "enabled", + .shape = CBM_JSON_LIKE_VALUE_LITERAL, + .expected_string = NULL, + .flags = CBM_JSON_LIKE_FIELD_REQUIRED}, + }; + ASSERT_EQ(jl_match_openhands_entry(ours, no_token, 2U, &captured), -1); + const cbm_json_like_object_field_t captured_literal[] = { + {.key = "enabled", + .shape = CBM_JSON_LIKE_VALUE_LITERAL, + .expected_string = "true", + .flags = CBM_JSON_LIKE_FIELD_REQUIRED | CBM_JSON_LIKE_FIELD_CAPTURE_STRING}, + }; + ASSERT_EQ(jl_match_openhands_entry(ours, captured_literal, 1U, &captured), -1); + PASS(); +} + +/* An OpenHands profile with an existing single-line list (#1826): the added + * name follows the list's own spacing, and removing it again restores the + * original bytes exactly — no stray space before the closing bracket. */ +static bool jl_round_trips(jl_fixture_t *fixture, const char *original, const char *expected) { + if (jl_write(fixture->path, original) != 0 || + cbm_json_like_add_unique_string(fixture->path, "mcp_server_refs", "codebase-memory-mcp") != + 0) { + return false; + } + char *added = jl_read(fixture->path); + bool ok = added && strcmp(added, expected) == 0; + free(added); + if (!ok || + cbm_json_like_remove_string(fixture->path, "mcp_server_refs", "codebase-memory-mcp") != 0) { + return false; + } + char *restored = jl_read(fixture->path); + ok = restored && strcmp(restored, original) == 0; + free(restored); + return ok; +} + +TEST(config_json_like_single_line_array_round_trips_byte_for_byte_issue1826) { + jl_fixture_t fixture; + ASSERT_EQ(jl_fixture_open(&fixture), 0); + ASSERT_TRUE(jl_round_trips( + &fixture, "{\"name\": \"custom\", \"mcp_server_refs\": [\"other-mcp\"]}\n", + "{\"name\": \"custom\", \"mcp_server_refs\": [\"other-mcp\", \"codebase-memory-mcp\"]}\n")); + ASSERT_TRUE( + jl_round_trips(&fixture, "{ \"mcp_server_refs\": [ \"other-mcp\" ] }\n", + "{ \"mcp_server_refs\": [ \"other-mcp\", \"codebase-memory-mcp\" ] }\n")); + ASSERT_TRUE(jl_round_trips(&fixture, "{\"mcp_server_refs\": [ ]}\n", + "{\"mcp_server_refs\": [ \"codebase-memory-mcp\" ]}\n")); + jl_fixture_close(&fixture); + PASS(); +} + TEST(config_json_like_top_level_array_create_escape_and_fail_closed) { jl_fixture_t fixture; ASSERT_EQ(jl_fixture_open(&fixture), 0); @@ -1025,6 +1195,9 @@ SUITE(config_json_like) { RUN_TEST(config_json_like_removes_first_middle_last_and_only); RUN_TEST(config_json_like_removal_preserves_comments_and_siblings); RUN_TEST(config_json_like_top_level_array_unique_string); + RUN_TEST(config_json_like_array_add_replaces_null_value_issue1826); + RUN_TEST(config_json_like_match_object_entry_literal_shape_issue1826); + RUN_TEST(config_json_like_single_line_array_round_trips_byte_for_byte_issue1826); RUN_TEST(config_json_like_top_level_array_create_escape_and_fail_closed); RUN_TEST(config_json_like_nested_array_creates_missing_path_and_escapes); RUN_TEST(config_json_like_nested_array_preserves_jsonc_and_is_idempotent);