Merge pull request #2896 from DeusData/fix/hermes-remover-empty-hooks

fix(cli): accept the emptied hooks: {} mapping in the Hermes hook editor

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) {

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

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);