From ae578bc1cc32bb8f159a69c486f938ad745c643f Mon Sep 17 00:00:00 2001 From: Martin Vogel Date: Fri, 25 Sep 2026 22:56:39 +0200 Subject: [PATCH] fix(install): Hermes hook edit no longer refuses unrelated YAML sections (#2209) `install` on a Hermes Agent config wrote the mcp_servers entry and the skill, then failed op=pre_llm_hook_install on the same config.yaml. The stock Hermes config (cli-config.yaml.example) reproduces it on its own. Root cause: the nested-sequence editor behind the hooks.pre_llm_call item validated EVERY line of the document and refused any flow collection or block scalar anywhere. The stock config carries flow sequences in sections the hook edit never reads or rewrites (`platform_toolsets:` -> `cli: [hermes-cli]`, reporters' `plugins:` -> `enabled: [...]`). The mapping-entry editor that writes mcp_servers into the same file already treats other top-level sections as opaque user content, which is why that op succeeded. Fix: yaml_sequence_validate_document now validates only the top-level section the edit descends into (the first sequence_path key). The root mapping is still validated at parse time, so section boundaries stay sound. An absent section is appended as a new top-level key and touches no existing line. Inside the edited `hooks` subtree the fail-closed rules are unchanged. This also unblocks uninstall (op=pre_llm_hook_uninstall), which shares the analyzer. Tests: new config_yaml_edit case with foreign flow sequences plus `|`/`>` block scalars (install, byte-idempotent reinstall, remove). The two #1924 "still refuses" pins now place the unsupported construct inside `hooks:`, where the refusal still applies byte-identically. Signed-off-by: Martin Vogel --- src/cli/config_yaml_edit.c | 26 +++++++++++-- tests/test_config_yaml_edit.c | 72 ++++++++++++++++++++++++++++++----- 2 files changed, 86 insertions(+), 12 deletions(-) diff --git a/src/cli/config_yaml_edit.c b/src/cli/config_yaml_edit.c index 38b3b4e33c..a6f8ced105 100644 --- a/src/cli/config_yaml_edit.c +++ b/src/cli/config_yaml_edit.c @@ -2698,8 +2698,28 @@ static int yaml_sequence_line_has_unsupported(const yaml_doc_t *doc, const yaml_ return quote != '\0' ? YAML_MATCH : 0; } -static int yaml_sequence_validate_document(const yaml_doc_t *doc) { - for (size_t i = 0U; i < doc->line_count; i++) { +/* Validates only the top-level section the edit descends into. The root + * mapping itself was already validated when the document was parsed, so the + * section boundaries are sound; every OTHER top-level section is opaque user + * content, the same contract the mapping-entry editor applies. Validating the + * whole document refused the stock Hermes config for flow sequences such as + * `cli: [hermes-cli]` under `platform_toolsets:`, which the hook edit never + * reads or rewrites (#2209). An absent root key means the sequence is appended + * as a new top-level section, which touches no existing line. */ +static int yaml_sequence_validate_document(const yaml_doc_t *doc, const char *root_key, + size_t root_key_len) { + bool found = false; + size_t section_line = 0U; + size_t section_colon = 0U; + if (yaml_find_unique_key(doc, 0U, root_key, root_key_len, &found, §ion_line, + §ion_colon) != 0) { + return YAML_ERROR; + } + if (!found) { + return 0; + } + size_t section_end = yaml_top_level_section_end(doc, section_line); + for (size_t i = section_line; i < section_end; i++) { const yaml_line_t *line = &doc->lines[i]; if (line->blank || line->comment || line->dquote_cont) { continue; @@ -2947,7 +2967,7 @@ static int yaml_sequence_analyze(const yaml_doc_t *doc, const char *const *seque const char *identity_key, const char *identity_value, yaml_mapping_sequence_target_t *target) { memset(target, 0, sizeof(*target)); - if (yaml_sequence_validate_document(doc) != 0) { + if (yaml_sequence_validate_document(doc, sequence_path[0], path_lengths[0]) != 0) { return YAML_ERROR; } size_t parent_begin = 0U; diff --git a/tests/test_config_yaml_edit.c b/tests/test_config_yaml_edit.c index f6f80d47ea..9efaa2e389 100644 --- a/tests/test_config_yaml_edit.c +++ b/tests/test_config_yaml_edit.c @@ -1228,12 +1228,14 @@ TEST(config_yaml_edit_hermes_accepts_interior_brackets_in_plain_scalar_issue1924 TEST(config_yaml_edit_hermes_hook_still_refuses_block_scalar_value_issue1924) { /* A `>` or `|` that BEGINS a value is a real block scalar whose - * indentation contract this editor does not model: the fail-closed - * refusal stays, and the file stays byte-identical. */ + * indentation contract this editor does not model: inside the `hooks` + * subtree the edit descends into, the fail-closed refusal stays and the + * file stays byte-identical. The same construct in an untouched section + * is opaque user content (#2209). */ static const char *const initials[] = { - "agent:\n personalities:\n probe: >\n folded face\n", - "agent:\n personalities:\n probe: |\n literal face\n", - "agent:\n personalities:\n probe: >-\n chomped face\n", + "hooks:\n probe: >\n folded face\n", + "hooks:\n probe: |\n literal face\n", + "hooks:\n probe: >-\n chomped face\n", }; for (size_t i = 0U; i < sizeof(initials) / sizeof(initials[0]); i++) { yaml_fixture_t fixture; @@ -1250,10 +1252,10 @@ TEST(config_yaml_edit_hermes_hook_still_refuses_block_scalar_value_issue1924) { TEST(config_yaml_edit_hermes_hook_still_refuses_nonempty_flow_sequence_value_issue1924) { /* Pins the #1631 line: exact `[]` is accepted, a non-empty flow sequence - * value is still unsupported in the sequence-document scan. */ - const char *initial = "agent:\n" - " personalities:\n" - " probe: [a, b]\n"; + * value inside the edited `hooks` subtree is still unsupported. Outside + * it, the value is opaque user content (#2209). */ + const char *initial = "hooks:\n" + " probe: [a, b]\n"; yaml_fixture_t fixture; ASSERT_EQ(yaml_fixture_init(&fixture, initial), 0); ASSERT_EQ(yaml_hermes_hook_upsert(&fixture), CBM_YAML_IDENTITY_EDIT_ERROR); @@ -2027,6 +2029,57 @@ TEST(config_yaml_edit_remove_on_absent_config_is_a_no_op) { PASS(); } +/* #2209: the stock Hermes config carries flow sequences and block scalars in + * sections the hook edit never touches (`platform_toolsets:` -> `cli: + * [hermes-cli]`, `plugins:` -> `enabled: [...]`, `|` prompts). They are opaque + * user content, exactly as for the mapping-entry editor that writes + * `mcp_servers` into the same file; only the `hooks` subtree is validated. */ +TEST(config_yaml_edit_nested_sequence_ignores_foreign_flow_and_block_scalars_issue2209) { + const char *initial = "plugins:\n" + " enabled: [disk-cleanup]\n" + "platform_toolsets:\n" + " cli: [hermes-cli]\n" + " telegram: [hermes-telegram]\n" + "toolsets: [all]\n" + "agent:\n" + " system_prompt: |\n" + " You are helpful.\n" + " - keep: [this, text]\n" + " note: >\n" + " folded\n" + "model:\n" + " default: \"anthropic/claude-opus-4.6\"\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 *installed = yaml_read_alloc(fixture.path); + ASSERT_NOT_NULL(installed); + ASSERT_EQ(strncmp(installed, initial, strlen(initial)), 0); + ASSERT_NOT_NULL(strstr(installed, "hooks:\n" + " pre_llm_call:\n" + " - id: \"codebase-memory-mcp\"\n")); + 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 *second = yaml_read_alloc(fixture.path); + ASSERT_NOT_NULL(second); + ASSERT_STR_EQ(second, installed); + free(second); + free(installed); + 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 *removed = yaml_read_alloc(fixture.path); + ASSERT_NOT_NULL(removed); + ASSERT_NULL(strstr(removed, "codebase-memory-mcp")); + ASSERT_EQ(strncmp(removed, initial, strlen(initial)), 0); + free(removed); + th_cleanup(fixture.dir); + PASS(); +} + TEST(config_yaml_edit_nested_sequence_ambiguity_fails_byte_identically) { const char *cases[] = { "hooks:\n pre_llm_call:\n - id: \"bad-indent\"\n", @@ -2192,6 +2245,7 @@ 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_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); #ifndef _WIN32