Merge pull request #2059 from Balneario-de-Cofrentes/fix/yaml-remove-absent-config

fix(cli): YAML removers answer OK for an absent config, so uninstall no longer refuses
This commit is contained in:
Martin Vogel
2026-09-07 01:13:46 +02:00
committed by GitHub
2 changed files with 83 additions and 0 deletions
+50
View File
@@ -3601,8 +3601,49 @@ int cbm_yaml_upsert_mapping_entry(const char *file_path, const char *section_key
return result == 0 && release_result == 0 ? 0 : YAML_ERROR;
}
/* Removal is idempotent: a config that does not exist has nothing to remove.
* The JSON removers already answer OK here (cbm_json_like_read_document's
* "missing" result); the YAML removers instead took the lock first, and the
* lock directory is created beside the target — so with the parent directory
* absent (`~/.hermes/` never created because Hermes was detected by its
* binary on PATH, not by its home) every uninstall reported
* "does not exist or cannot be inspected" and refused to remove the
* executable. Only a true ENOENT short-circuits: a dangling symlink still
* reaches the reader and its byte-identical rejection. */
static bool yaml_remove_target_absent(const char *path) {
if (!path) {
return false;
}
#ifdef _WIN32
/* FILE_FLAG_OPEN_REPARSE_POINT, as in toml_read_file: opens the link
* itself rather than following it, so a dangling symlink does NOT count as
* absent and still reaches the reader — matching the POSIX branch, where
* lstat does not follow either. */
wchar_t *wide = cbm_utf8_to_wide(path);
if (!wide) {
return false;
}
HANDLE handle = CreateFileW(
wide, GENERIC_READ, FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, NULL,
OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL | FILE_FLAG_OPEN_REPARSE_POINT, NULL);
DWORD error = handle == INVALID_HANDLE_VALUE ? GetLastError() : 0;
free(wide);
if (handle != INVALID_HANDLE_VALUE) {
CloseHandle(handle);
return false;
}
return error == ERROR_FILE_NOT_FOUND || error == ERROR_PATH_NOT_FOUND;
#else
struct stat path_state;
return lstat(path, &path_state) != 0 && errno == ENOENT;
#endif
}
int cbm_yaml_remove_mapping_entry(const char *file_path, const char *section_key,
const char *entry_key) {
if (yaml_remove_target_absent(file_path)) {
return 0;
}
yaml_config_lock_t lock;
if (yaml_lock_acquire(file_path, &lock) != 0) {
return YAML_ERROR;
@@ -3626,6 +3667,9 @@ int cbm_yaml_upsert_owned_mapping_entry(const char *file_path, const char *secti
int cbm_yaml_remove_owned_mapping_entry(const char *file_path, const char *section_key,
const char *entry_key, const char *canonical_entry_block) {
if (yaml_remove_target_absent(file_path)) {
return CBM_YAML_IDENTITY_EDIT_OK;
}
yaml_config_lock_t lock;
if (yaml_lock_acquire(file_path, &lock) != 0) {
return CBM_YAML_IDENTITY_EDIT_ERROR;
@@ -3653,6 +3697,9 @@ int cbm_yaml_upsert_mapping_sequence_item(const char *file_path, const char *con
int cbm_yaml_remove_mapping_sequence_item(const char *file_path, const char *const *sequence_path,
size_t sequence_path_len, const char *identity_key,
const char *identity_scalar, const char *canonical_item) {
if (yaml_remove_target_absent(file_path)) {
return CBM_YAML_IDENTITY_EDIT_OK;
}
yaml_config_lock_t lock;
if (yaml_lock_acquire(file_path, &lock) != 0) {
return CBM_YAML_IDENTITY_EDIT_ERROR;
@@ -3675,6 +3722,9 @@ int cbm_yaml_upsert_string_list_item(const char *file_path, const char *key, con
}
int cbm_yaml_remove_string_list_item(const char *file_path, const char *key, const char *item) {
if (yaml_remove_target_absent(file_path)) {
return 0;
}
yaml_config_lock_t lock;
if (yaml_lock_acquire(file_path, &lock) != 0) {
return YAML_ERROR;
+33
View File
@@ -2,6 +2,7 @@
* test_config_yaml_edit.c — Conservative YAML config editor tests.
*/
#include "../src/foundation/compat.h"
#include "../src/foundation/compat_fs.h"
#include "test_framework.h"
#include "test_helpers.h"
@@ -1826,6 +1827,37 @@ TEST(config_yaml_edit_nested_sequence_removes_only_exact_canonical_item) {
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
* beside a file whose directory does not exist, which `uninstall` then
* counted as an agent cleanup error and refused to remove the executable.
* Every remover answers OK for a genuinely absent path and creates nothing.
* Runs on every platform: the fix has a separate Windows branch. */
TEST(config_yaml_edit_remove_on_absent_config_is_a_no_op) {
yaml_fixture_t fixture;
ASSERT_EQ(yaml_fixture_init(&fixture, NULL), 0);
char absent[sizeof(fixture.path) + 32U];
ASSERT(snprintf(absent, sizeof(absent), "%s/no-such-dir/config.yaml", fixture.dir) > 0);
ASSERT_EQ(cbm_yaml_remove_mapping_sequence_item(absent, yaml_hook_sequence_path, 2U, "id",
yaml_hook_identity, yaml_hook_canonical_item),
CBM_YAML_IDENTITY_EDIT_OK);
ASSERT_EQ(cbm_yaml_remove_owned_mapping_entry(absent, "mcp_servers", "codebase-memory-mcp",
" command: \"/opt/cbm\"\n"),
CBM_YAML_IDENTITY_EDIT_OK);
ASSERT_EQ(cbm_yaml_remove_mapping_entry(absent, "hooks", "pre_llm_call"), 0);
ASSERT_EQ(cbm_yaml_remove_string_list_item(absent, "read", "AGENTS.md"), 0);
/* Nothing was created. cbm_path_info_utf8 rather than lstat: same meaning
* — it reports the link instead of following it — and it compiles on
* Windows, which is exactly where the fix has its own branch. */
cbm_path_info_t absent_info;
ASSERT(cbm_path_info_utf8(absent, &absent_info) != 0);
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",
@@ -1980,6 +2012,7 @@ SUITE(config_yaml_edit) {
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_ambiguity_fails_byte_identically);
RUN_TEST(config_yaml_edit_remove_on_absent_config_is_a_no_op);
#ifndef _WIN32
RUN_TEST(config_yaml_edit_accepts_interior_asterisk_in_plain_scalar_issue1631);
RUN_TEST(config_yaml_edit_still_refuses_leading_alias_issue1631);