diff --git a/src/cli/config_yaml_edit.c b/src/cli/config_yaml_edit.c index a6f8ced10..09a079212 100644 --- a/src/cli/config_yaml_edit.c +++ b/src/cli/config_yaml_edit.c @@ -2589,6 +2589,10 @@ typedef struct { bool identity_found; size_t identity_start; size_t identity_end; + /* The missing rest of the path begins under a `parent: {}` header. */ + bool parent_inline_empty; + size_t parent_line; + size_t parent_colon; } yaml_mapping_sequence_target_t; static void yaml_sequence_key_vec_free(yaml_sequence_key_vec_t *keys) { @@ -2962,6 +2966,37 @@ static int yaml_sequence_parse_block(const yaml_doc_t *doc, size_t begin_line, s return 0; } +/* `section: {}` is how the mapping-entry editor writes a section it emptied + * (yaml_build_last_mapping_entry_removal), and its upsert accepts it back. On + * a PARENT of the sequence path it is that same empty mapping, so the rest of + * the path is missing beneath it: removal is a no-op and upsert expands the + * header in place. The Hermes remover leaves `hooks: {}` whenever our hook was + * the only entry. The sequence key itself holding `{}`, any other flow value, + * and content nested under the inline mapping stay refused. */ +static int yaml_sequence_target_empty_parent(const yaml_doc_t *doc, size_t key_line, size_t colon, + size_t child_end, size_t next_depth, + size_t sequence_path_len, + yaml_mapping_sequence_target_t *target) { + bool inline_empty = false; + if (next_depth >= sequence_path_len || + yaml_tail_is_explicit_empty_mapping(doc->data, colon, doc->lines[key_line].text_end, + &inline_empty) != 0 || + !inline_empty) { + return YAML_ERROR; + } + for (size_t i = key_line + YAML_UNIT; i < child_end; i++) { + if (!doc->lines[i].blank && !doc->lines[i].comment) { + return YAML_ERROR; + } + } + target->missing_index = next_depth; + target->insert_offset = yaml_sequence_line_offset(doc, child_end); + target->parent_inline_empty = true; + target->parent_line = key_line; + target->parent_colon = colon; + return 0; +} + static int yaml_sequence_analyze(const yaml_doc_t *doc, const char *const *sequence_path, const size_t *path_lengths, size_t sequence_path_len, const char *identity_key, const char *identity_value, @@ -2993,10 +3028,11 @@ static int yaml_sequence_analyze(const yaml_doc_t *doc, const char *const *seque return 0; } const yaml_line_t *header = &doc->lines[key_line]; - if (!yaml_tail_is_empty(doc->data, colon, header->text_end)) { - return YAML_ERROR; - } size_t child_end = yaml_sequence_nested_end(doc, key_line, parent_end); + if (!yaml_tail_is_empty(doc->data, colon, header->text_end)) { + return yaml_sequence_target_empty_parent(doc, key_line, colon, child_end, + depth + YAML_UNIT, sequence_path_len, target); + } if (depth + YAML_UNIT == sequence_path_len) { target->sequence_found = true; target->missing_index = sequence_path_len; @@ -3095,13 +3131,52 @@ static int yaml_sequence_validate_canonical(const char *canonical_item, size_t c return result; } +/* Copies the document up to insert_offset with the `parent: {}` header turned + * into a block header `parent:`, exactly as + * yaml_build_inline_empty_mapping_section_add does for a mapping section: a + * trailing comment and the line ending are kept. */ +static int yaml_sequence_append_expanded_parent(yaml_buf_t *out, const yaml_doc_t *doc, + const yaml_mapping_sequence_target_t *target) { + if (target->parent_line >= doc->line_count) { + return YAML_ERROR; + } + const yaml_line_t *header = &doc->lines[target->parent_line]; + size_t comment = 0U; + if (target->insert_offset < header->end || + yaml_find_comment(doc->data, target->parent_colon + YAML_UNIT, header->text_end, + &comment) != 0 || + yaml_buf_append(out, doc->data, target->parent_colon + YAML_UNIT) != 0) { + return YAML_ERROR; + } + size_t kept_tail = header->text_end; + if (comment < header->text_end) { + if (yaml_buf_append_char(out, ' ') != 0) { + return YAML_ERROR; + } + kept_tail = comment; + } + if (yaml_buf_append(out, doc->data + kept_tail, header->end - kept_tail) != 0 || + yaml_buf_append(out, doc->data + header->end, target->insert_offset - header->end) != 0 || + (out->len > 0U && out->data[out->len - YAML_UNIT] != '\n' && + yaml_buf_append(out, doc->eol, doc->eol_len) != 0)) { + return YAML_ERROR; + } + return 0; +} + static int yaml_sequence_build_insert(yaml_buf_t *out, const yaml_doc_t *doc, const yaml_mapping_sequence_target_t *target, const char *const *sequence_path, const size_t *path_lengths, size_t sequence_path_len, const yaml_buf_t *rendered_item) { - if (target->insert_offset > doc->len || - yaml_buf_append(out, doc->data, target->insert_offset) != 0 || - yaml_append_separator_if_needed(out, doc, target->insert_offset) != 0) { + if (target->insert_offset > doc->len) { + return YAML_ERROR; + } + if (target->parent_inline_empty) { + if (yaml_sequence_append_expanded_parent(out, doc, target) != 0) { + return YAML_ERROR; + } + } else if (yaml_buf_append(out, doc->data, target->insert_offset) != 0 || + yaml_append_separator_if_needed(out, doc, target->insert_offset) != 0) { return YAML_ERROR; } if (!target->sequence_found) { diff --git a/tests/test_cli.c b/tests/test_cli.c index 3284ef72a..15255ff84 100644 --- a/tests/test_cli.c +++ b/tests/test_cli.c @@ -9450,6 +9450,143 @@ TEST(cli_hermes_stable_shell_context_contract) { PASS(); } +/* A fake HOME holding only ~/.hermes/config.yaml, so install and uninstall + * reach exactly the Hermes editors. */ +typedef struct { + char tmpdir[256]; + char config_path[640]; + char binary_path[640]; + char *saved_home; + char *saved_path; + char *saved_hermes; +} cli_hermes_cycle_t; + +static bool cli_hermes_cycle_open(cli_hermes_cycle_t *cycle, const char *initial_config) { + memset(cycle, 0, sizeof(*cycle)); + snprintf(cycle->tmpdir, sizeof(cycle->tmpdir), "/tmp/cli-hermes-cycle-XXXXXX"); + if (!cbm_mkdtemp(cycle->tmpdir)) { + cycle->tmpdir[0] = '\0'; + return false; + } + cycle->saved_home = save_test_env("HOME"); + cycle->saved_path = save_test_env("PATH"); + cycle->saved_hermes = save_test_env("HERMES_HOME"); + cbm_setenv("HOME", cycle->tmpdir, 1); + cbm_setenv("PATH", cycle->tmpdir, 1); + cbm_unsetenv("HERMES_HOME"); + + char hermes_dir[512]; + snprintf(hermes_dir, sizeof(hermes_dir), "%s/.hermes", cycle->tmpdir); + snprintf(cycle->config_path, sizeof(cycle->config_path), "%s/config.yaml", hermes_dir); +#ifdef _WIN32 + snprintf(cycle->binary_path, sizeof(cycle->binary_path), + "%s/.local/bin/codebase-memory-mcp.exe", cycle->tmpdir); +#else + snprintf(cycle->binary_path, sizeof(cycle->binary_path), "%s/.local/bin/codebase-memory-mcp", + cycle->tmpdir); +#endif + test_mkdirp(hermes_dir); + return write_test_file(cycle->config_path, initial_config) == 0; +} + +static void cli_hermes_cycle_close(cli_hermes_cycle_t *cycle) { + if (!cycle->tmpdir[0]) { + return; + } + restore_test_env("HOME", cycle->saved_home); + restore_test_env("PATH", cycle->saved_path); + restore_test_env("HERMES_HOME", cycle->saved_hermes); + test_rmdir_r(cycle->tmpdir); + cycle->tmpdir[0] = '\0'; +} + +/* Found while finishing #1862: when our hook was the only entry under + * `hooks:`, uninstall empties that mapping and the mapping editor writes it as + * `hooks: {}` (the same convention as `mcp_servers: {}`). The hook sequence + * editor then refused the file, so install -> uninstall -> install exited 1, + * and so did a repeated uninstall. The explicit empty mapping must be accepted: + * a repeated uninstall changes nothing and the reinstall restores the first + * install byte for byte, with exactly one owned hook. */ +TEST(cli_hermes_hook_reinstalls_after_uninstall_empties_hooks) { + cli_hermes_cycle_t cycle; + if (!cli_hermes_cycle_open(&cycle, "theme: solarized\n")) { + cli_hermes_cycle_close(&cycle); + FAIL("Hermes fake-HOME setup failed"); + } + char *argv[] = {"uninstall", "--yes"}; + int install_rc = cbm_install_agent_configs(cycle.tmpdir, cycle.binary_path, false, false); + char *installed = read_test_file_alloc(cycle.config_path); + int uninstall_rc = cli_test_cmd_uninstall(2, argv); + char *emptied = read_test_file_alloc(cycle.config_path); + int repeat_rc = cli_test_cmd_uninstall(2, argv); + char *repeated = read_test_file_alloc(cycle.config_path); + int reinstall_rc = cbm_install_agent_configs(cycle.tmpdir, cycle.binary_path, false, false); + char *reinstalled = read_test_file_alloc(cycle.config_path); + cli_hermes_cycle_close(&cycle); + + bool installed_once = install_rc == 0 && installed && + test_count_substring(installed, "pre_llm_call:") == 1U && + test_count_substring(installed, "hook-augment") == 1U; + bool emptied_clean = uninstall_rc == 0 && emptied && strstr(emptied, "theme: solarized\n") && + test_count_substring(emptied, "hooks: {}\n") == 1U && + !strstr(emptied, "pre_llm_call") && !strstr(emptied, "hook-augment"); + bool repeat_unchanged = repeat_rc == 0 && emptied && repeated && strcmp(repeated, emptied) == 0; + bool reinstalled_once = + reinstall_rc == 0 && installed && reinstalled && strcmp(reinstalled, installed) == 0; + free(installed); + free(emptied); + free(repeated); + free(reinstalled); + if (!installed_once) + FAIL("Hermes install must write exactly one owned pre_llm_call hook"); + if (!emptied_clean) + FAIL("uninstall must remove the owned hook and leave only the empty hooks: {} mapping"); + if (!repeat_unchanged) + FAIL("a repeated uninstall must accept hooks: {} and leave the file unchanged"); + if (!reinstalled_once) + FAIL("reinstall must accept hooks: {} and restore exactly one owned hook"); + PASS(); +} + +/* The same cycle with a user's own hook in the shared pre_llm_call sequence: + * the foreign hook survives untouched and uninstall restores the original + * file byte for byte. */ +TEST(cli_hermes_hook_cycle_keeps_foreign_pre_llm_hook) { + static const char original[] = "theme: solarized\n" + "mcp_servers: {}\n" + "hooks:\n" + " pre_llm_call:\n" + " - command: \"/usr/bin/user-hermes-hook\"\n"; + cli_hermes_cycle_t cycle; + if (!cli_hermes_cycle_open(&cycle, original)) { + cli_hermes_cycle_close(&cycle); + FAIL("Hermes fake-HOME setup failed"); + } + char *argv[] = {"uninstall", "--yes"}; + int install_rc = cbm_install_agent_configs(cycle.tmpdir, cycle.binary_path, false, false); + char *installed = read_test_file_alloc(cycle.config_path); + int uninstall_rc = cli_test_cmd_uninstall(2, argv); + char *restored = read_test_file_alloc(cycle.config_path); + int reinstall_rc = cbm_install_agent_configs(cycle.tmpdir, cycle.binary_path, false, false); + char *reinstalled = read_test_file_alloc(cycle.config_path); + cli_hermes_cycle_close(&cycle); + + bool merged = install_rc == 0 && installed && + test_count_substring(installed, "/usr/bin/user-hermes-hook") == 1U && + test_count_substring(installed, "pre_llm_call:") == 1U && + test_count_substring(installed, "hook-augment") == 1U; + bool restored_exactly = uninstall_rc == 0 && restored && strcmp(restored, original) == 0; + bool reinstalled_once = + reinstall_rc == 0 && installed && reinstalled && strcmp(reinstalled, installed) == 0; + free(installed); + free(restored); + free(reinstalled); + if (!merged || !restored_exactly || !reinstalled_once) + FAIL("a user's own pre_llm_call hook must survive install, uninstall and reinstall, and " + "uninstall must restore the original file byte for byte"); + PASS(); +} + #ifndef _WIN32 TEST(cli_detected_agent_summary_includes_registry_clients) { char tmpdir[256]; @@ -20553,6 +20690,8 @@ SUITE(cli) { RUN_TEST(cli_junie_current_durable_context_contract); RUN_TEST(cli_rovo_installs_documented_global_memory); RUN_TEST(cli_hermes_stable_shell_context_contract); + RUN_TEST(cli_hermes_hook_reinstalls_after_uninstall_empties_hooks); + RUN_TEST(cli_hermes_hook_cycle_keeps_foreign_pre_llm_hook); #ifndef _WIN32 RUN_TEST(cli_detected_agent_summary_includes_registry_clients); #endif diff --git a/tests/test_config_yaml_edit.c b/tests/test_config_yaml_edit.c index 9efaa2e38..95bc89404 100644 --- a/tests/test_config_yaml_edit.c +++ b/tests/test_config_yaml_edit.c @@ -1998,6 +1998,129 @@ TEST(config_yaml_edit_nested_sequence_removes_only_exact_canonical_item) { PASS(); } +/* Found while finishing #1862: cbm_yaml_remove_mapping_entry writes a section + * it empties as `section: {}`, and the Hermes remover empties `hooks:` whenever + * our hook was its only entry. The sequence editor refused that explicit empty + * parent mapping, so the next install (and a repeated removal) failed. The + * primitives' own round trip must land back on the first insert. */ +TEST(config_yaml_edit_nested_sequence_reinserts_under_emptied_parent) { + const char *initial = "theme: x\n"; + yaml_fixture_t fixture; + ASSERT_EQ(yaml_fixture_init(&fixture, initial), 0); + ASSERT_EQ(cbm_yaml_upsert_mapping_sequence_item(fixture.path, yaml_hook_sequence_path, 2U, "id", + yaml_hook_identity, yaml_hook_canonical_item), + CBM_YAML_IDENTITY_EDIT_OK); + char *first = yaml_read_alloc(fixture.path); + ASSERT_NOT_NULL(first); + ASSERT_EQ(cbm_yaml_remove_mapping_sequence_item(fixture.path, yaml_hook_sequence_path, 2U, "id", + yaml_hook_identity, yaml_hook_canonical_item), + CBM_YAML_IDENTITY_EDIT_OK); + ASSERT_EQ(cbm_yaml_remove_mapping_entry(fixture.path, "hooks", "pre_llm_call"), 0); + char *emptied = yaml_read_alloc(fixture.path); + ASSERT_NOT_NULL(emptied); + ASSERT_STR_EQ(emptied, "theme: x\nhooks: {}\n"); + free(emptied); + ASSERT_EQ(cbm_yaml_remove_mapping_sequence_item(fixture.path, yaml_hook_sequence_path, 2U, "id", + yaml_hook_identity, yaml_hook_canonical_item), + CBM_YAML_IDENTITY_EDIT_OK); + ASSERT_EQ(cbm_yaml_upsert_mapping_sequence_item(fixture.path, yaml_hook_sequence_path, 2U, "id", + yaml_hook_identity, yaml_hook_canonical_item), + CBM_YAML_IDENTITY_EDIT_OK); + char *again = yaml_read_alloc(fixture.path); + ASSERT_NOT_NULL(again); + ASSERT_STR_EQ(again, first); + free(again); + free(first); + th_cleanup(fixture.dir); + PASS(); +} + +/* An exact empty flow mapping on a PARENT of the sequence path is an empty + * mapping, the same `section: {}` the mapping-entry editor writes and accepts. + * Removal is a byte-identical no-op; upsert expands the header in place, + * keeping its comment, the line endings and every other byte. */ +TEST(config_yaml_edit_nested_sequence_expands_explicit_empty_parent_mapping) { + static const char item_lf[] = " pre_llm_call:\n" + " - id: \"codebase-memory-mcp\"\n" + " type: \"command\"\n" + " command: \"/opt/Codebase Memory/bin/cbm\"\n"; + static const char item_crlf[] = " pre_llm_call:\r\n" + " - id: \"codebase-memory-mcp\"\r\n" + " type: \"command\"\r\n" + " command: \"/opt/Codebase Memory/bin/cbm\"\r\n"; + struct { + const char *initial; + const char *header; + const char *item; + const char *tail; + } cases[] = { + {"theme: x\nhooks: {} # keep me\nafter: 1\n", "theme: x\nhooks: # keep me\n", item_lf, + "after: 1\n"}, + {"theme: x\r\nhooks: {}\r\nafter: 1\r\n", "theme: x\r\nhooks:\r\n", item_crlf, + "after: 1\r\n"}, + {"theme: x\nhooks: {}", "theme: x\nhooks:\n", item_lf, ""}, + {"hooks: {}\n# trailing note\n", "hooks:\n# trailing note\n", item_lf, ""}, + }; + for (size_t i = 0U; i < sizeof(cases) / sizeof(cases[0]); i++) { + char expected[1024]; + ASSERT(snprintf(expected, sizeof(expected), "%s%s%s", cases[i].header, cases[i].item, + cases[i].tail) > 0); + yaml_fixture_t fixture; + ASSERT_EQ(yaml_fixture_init(&fixture, cases[i].initial), 0); + ASSERT_EQ(cbm_yaml_remove_mapping_sequence_item(fixture.path, yaml_hook_sequence_path, 2U, + "id", yaml_hook_identity, + yaml_hook_canonical_item), + CBM_YAML_IDENTITY_EDIT_OK); + char *unchanged = yaml_read_alloc(fixture.path); + ASSERT_NOT_NULL(unchanged); + ASSERT_STR_EQ(unchanged, cases[i].initial); + free(unchanged); + for (int pass = 0; pass < 2; pass++) { + ASSERT_EQ(cbm_yaml_upsert_mapping_sequence_item(fixture.path, yaml_hook_sequence_path, + 2U, "id", yaml_hook_identity, + yaml_hook_canonical_item), + CBM_YAML_IDENTITY_EDIT_OK); + char *expanded = yaml_read_alloc(fixture.path); + ASSERT_NOT_NULL(expanded); + ASSERT_STR_EQ(expanded, expected); + free(expanded); + } + th_cleanup(fixture.dir); + } + PASS(); +} + +/* Only an exact `{}` on a parent with nothing nested beneath it is accepted. + * The sequence key itself holding a mapping, a non-empty flow mapping, a flow + * sequence where a mapping belongs, and content indented under an inline `{}` + * stay refused byte-identically by both upsert and removal. */ +TEST(config_yaml_edit_nested_sequence_still_refuses_non_empty_parent_flow) { + const char *cases[] = { + "hooks:\n pre_llm_call: {}\n", + "hooks: {pre_llm_call: []}\n", + "hooks: []\n", + "hooks: {}\n pre_llm_call:\n - id: \"codebase-memory-mcp\"\n", + }; + for (size_t i = 0U; i < sizeof(cases) / sizeof(cases[0]); i++) { + yaml_fixture_t fixture; + ASSERT_EQ(yaml_fixture_init(&fixture, cases[i]), 0); + ASSERT_EQ(cbm_yaml_upsert_mapping_sequence_item(fixture.path, yaml_hook_sequence_path, 2U, + "id", yaml_hook_identity, + yaml_hook_canonical_item), + CBM_YAML_IDENTITY_EDIT_ERROR); + ASSERT_EQ(cbm_yaml_remove_mapping_sequence_item(fixture.path, yaml_hook_sequence_path, 2U, + "id", yaml_hook_identity, + yaml_hook_canonical_item), + CBM_YAML_IDENTITY_EDIT_ERROR); + char *after = yaml_read_alloc(fixture.path); + ASSERT_NOT_NULL(after); + ASSERT_STR_EQ(after, cases[i]); + free(after); + th_cleanup(fixture.dir); + } + PASS(); +} + /* Uninstall must be idempotent against a config that was never written. Hermes * is detected by its binary on PATH, so a HOME without `.hermes/` reached the * YAML removers with an absent target — and they failed acquiring the lock @@ -2245,6 +2368,9 @@ SUITE(config_yaml_edit) { RUN_TEST(config_yaml_edit_nested_sequence_preserves_crlf); RUN_TEST(config_yaml_edit_nested_sequence_foreign_identity_is_preserved); RUN_TEST(config_yaml_edit_nested_sequence_removes_only_exact_canonical_item); + RUN_TEST(config_yaml_edit_nested_sequence_reinserts_under_emptied_parent); + RUN_TEST(config_yaml_edit_nested_sequence_expands_explicit_empty_parent_mapping); + RUN_TEST(config_yaml_edit_nested_sequence_still_refuses_non_empty_parent_flow); RUN_TEST(config_yaml_edit_nested_sequence_ignores_foreign_flow_and_block_scalars_issue2209); RUN_TEST(config_yaml_edit_nested_sequence_ambiguity_fails_byte_identically); RUN_TEST(config_yaml_edit_remove_on_absent_config_is_a_no_op);