From 0cf0ccb526cd5e396a8687460edb012ca1a7bcdf Mon Sep 17 00:00:00 2001 From: astandrik Date: Wed, 5 Aug 2026 12:42:58 +0300 Subject: [PATCH] fix(cli): reconcile normalized Codex hooks Signed-off-by: astandrik --- src/cli/cli.c | 144 ++++++------ src/cli/config_toml_edit.c | 429 +++++++++++++++++++++++++++++++++- src/cli/config_toml_edit.h | 8 + tests/test_cli.c | 96 +++++++- tests/test_config_toml_edit.c | 137 +++++++++++ 5 files changed, 728 insertions(+), 86 deletions(-) diff --git a/src/cli/cli.c b/src/cli/cli.c index c58738a42..41909b6dd 100644 --- a/src/cli/cli.c +++ b/src/cli/cli.c @@ -3382,43 +3382,22 @@ bool cbm_optional_hook_supported_for_testing(const char *agent_name, bool window } #endif -static int cbm_upsert_codex_hooks_command(const char *config_path, const char *command, - const char *command_windows) { +static int cbm_reconcile_codex_hooks_command(const char *config_path, const char *command, + const char *command_windows, + cbm_toml_codex_hook_action_t action, bool check_only) { if (!config_path || !command || !command_windows) { return CLI_ERR; } - char escaped[CLI_BUF_8K]; - char escaped_windows[CLI_BUF_8K]; - if (cbm_toml_escape_basic_string(command, escaped, sizeof(escaped)) != CLI_OK || - cbm_toml_escape_basic_string(command_windows, escaped_windows, sizeof(escaped_windows)) != - CLI_OK) { - return CLI_ERR; - } - char block[CLI_BUF_8K]; - int written = snprintf(block, sizeof(block), - "[[hooks.SessionStart]]\n" - "matcher = \"startup|resume|clear|compact\"\n\n" - "[[hooks.SessionStart.hooks]]\n" - "type = \"command\"\n" - "command = \"%s\"\n" - "command_windows = \"%s\"\n" - "timeout = 5\n\n" - "[[hooks.SubagentStart]]\n" - "matcher = \"*\"\n\n" - "[[hooks.SubagentStart.hooks]]\n" - "type = \"command\"\n" - "command = \"%s\"\n" - "command_windows = \"%s\"\n" - "timeout = 5\n", - escaped, escaped_windows, escaped, escaped_windows); - if (written < 0 || (size_t)written >= sizeof(block)) { - return CLI_ERR; - } - return cbm_toml_upsert_managed_block(config_path, CODEX_HOOK_BEGIN, CODEX_HOOK_END, block) == 0 + return cbm_toml_reconcile_codex_hooks(config_path, CODEX_HOOK_BEGIN, CODEX_HOOK_END, command, + command_windows, action, check_only ? 1 : 0) == 0 ? CLI_OK : CLI_ERR; } - +static int cbm_upsert_codex_hooks_command(const char *config_path, const char *command, + const char *command_windows) { + return cbm_reconcile_codex_hooks_command(config_path, command, command_windows, + CBM_TOML_CODEX_HOOK_UPSERT, false); +} /* Public path used by config-level regression tests and manual callers. */ int cbm_upsert_codex_hooks(const char *config_path) { return cbm_upsert_codex_hooks_command(config_path, "codebase-memory-mcp hook-augment", @@ -3426,10 +3405,9 @@ int cbm_upsert_codex_hooks(const char *config_path) { } int cbm_remove_codex_hooks(const char *config_path) { - return config_path && - cbm_toml_remove_managed_block(config_path, CODEX_HOOK_BEGIN, CODEX_HOOK_END) == 0 - ? CLI_OK - : CLI_ERR; + return cbm_reconcile_codex_hooks_command(config_path, "codebase-memory-mcp hook-augment", + "codebase-memory-mcp hook-augment", + CBM_TOML_CODEX_HOOK_REMOVE, false); } /* ── OpenCode MCP config (JSON with "mcp" key) ───────────────── */ @@ -8166,6 +8144,23 @@ static void install_cli_agent_configs(const cbm_detected_agents_t *agents, const snprintf(ip, sizeof(ip), "%s/AGENTS.md", config_dir); snprintf(skills_dir, sizeof(skills_dir), "%s/skills", config_dir); snprintf(ap, sizeof(ap), "%s/agents/codebase-memory.toml", config_dir); + char command[CLI_BUF_8K]; + char command_windows[CLI_BUF_8K]; + char hooks_json[CLI_BUF_1K]; + snprintf(hooks_json, sizeof(hooks_json), "%s/hooks.json", config_dir); + bool use_hooks_json = cbm_file_exists(hooks_json); + bool commands_ok = + cbm_build_augment_command(binary_path, command, sizeof(command)) == CLI_OK && + cbm_build_augment_command_windows(binary_path, command_windows, + sizeof(command_windows)) == CLI_OK; + cbm_toml_codex_hook_action_t preflight_action = + use_hooks_json ? CBM_TOML_CODEX_HOOK_REMOVE : CBM_TOML_CODEX_HOOK_UPSERT; + if (!commands_ok || cbm_reconcile_codex_hooks_command(cp, command, command_windows, + preflight_action, true) != CLI_OK) { + record_agent_config_error(false, "Codex CLI", + commands_ok ? "hook_preflight" : "hook_command_build", cp); + goto codex_install_done; + } install_generic_agent_config("Codex CLI", binary_path, cp, ip, dry_run, cbm_upsert_codex_mcp); install_agent_skill("Codex CLI", skills_dir, force, dry_run); @@ -8183,53 +8178,29 @@ static void install_cli_agent_configs(const cbm_detected_agents_t *agents, const * SessionStart reminder there instead of config.toml. Writing both * makes Codex warn about loading hooks from two representations (#570). * config.toml remains the mcp_config target above either way. */ - char hooks_json[CLI_BUF_1K]; - snprintf(hooks_json, sizeof(hooks_json), "%s/hooks.json", config_dir); - bool use_hooks_json = cbm_file_exists(hooks_json); const char *hook_target = use_hooks_json ? hooks_json : cp; if (g_install_plan) { plan_record("Codex CLI", "hook", hook_target); } else { bool hook_ok = true; - if (!dry_run) { - char command[CLI_BUF_8K]; - char command_windows[CLI_BUF_8K]; - if (cbm_build_augment_command(binary_path, command, sizeof(command)) == CLI_OK && - cbm_build_augment_command_windows(binary_path, command_windows, - sizeof(command_windows)) == CLI_OK) { - if (use_hooks_json) { - if (cbm_upsert_paired_lifecycle_hooks_json( - hooks_json, command, command_windows, NULL, CMM_HOOK_TIMEOUT_SEC) == - CLI_OK) { - if (cbm_remove_codex_hooks(cp) != CLI_OK) { - hook_ok = false; - record_agent_config_error(false, "Codex CLI", "legacy_hook_cleanup", - cp); - } - } else { - hook_ok = false; - record_agent_config_error(false, "Codex CLI", "hook_install", - hooks_json); - } - } else { - if (cbm_upsert_codex_hooks_command(cp, command, command_windows) != - CLI_OK) { - hook_ok = false; - record_agent_config_error(false, "Codex CLI", "hook_install", cp); - } - } - } else { - hook_ok = false; - record_agent_config_error(false, "Codex CLI", "hook_command_build", - hook_target); - } + if (!dry_run && use_hooks_json) { + hook_ok = + cbm_upsert_paired_lifecycle_hooks_json(hooks_json, command, command_windows, + NULL, CMM_HOOK_TIMEOUT_SEC) == CLI_OK && + cbm_reconcile_codex_hooks_command(cp, command, command_windows, + CBM_TOML_CODEX_HOOK_REMOVE, false) == CLI_OK; + } else if (!dry_run) { + hook_ok = cbm_upsert_codex_hooks_command(cp, command, command_windows) == CLI_OK; } - if (hook_ok) { + if (!hook_ok) { + record_agent_config_error(false, "Codex CLI", "hook_install", hook_target); + } else { printf(" hooks: SessionStart + SubagentStart (dynamic graph context)\n"); + printf(" note: non-managed hooks require /hooks trust; definition changes " + "require re-trust\n"); } - printf(" note: non-managed hooks require /hooks trust; definition changes require " - "re-trust\n"); } + codex_install_done:; } if (agents->gemini) { install_gemini_config(home, binary_path, dry_run); @@ -10166,8 +10137,28 @@ static void uninstall_cli_agents(const cbm_detected_agents_t *agents, const char snprintf(skills_dir, sizeof(skills_dir), "%s/skills", config_dir); snprintf(ap, sizeof(ap), "%s/agents/codebase-memory.toml", config_dir); cbm_agent_installed_binary_path(home, installed_binary, sizeof(installed_binary)); + char hook_command[CLI_BUF_8K]; + char hook_command_windows[CLI_BUF_8K]; + bool hook_command_ok = cbm_build_augment_command(installed_binary, hook_command, + sizeof(hook_command)) == CLI_OK; + bool hook_preflight_ok = + hook_command_ok && + cbm_build_augment_command_windows(installed_binary, hook_command_windows, + sizeof(hook_command_windows)) == CLI_OK && + cbm_reconcile_codex_hooks_command(cp, hook_command, hook_command_windows, + CBM_TOML_CODEX_HOOK_REMOVE, true) == CLI_OK; + if (!hook_preflight_ok) { + record_agent_config_error(true, "Codex CLI", "hook_preflight", cp); + goto codex_toml_done; + } uninstall_agent_mcp_instr((mcp_uninstall_args_t){"Codex CLI", cp, ip}, dry_run, cbm_remove_codex_mcp_owned); + if (!dry_run && + cbm_reconcile_codex_hooks_command(cp, hook_command, hook_command_windows, + CBM_TOML_CODEX_HOOK_REMOVE, false) != CLI_OK) { + record_agent_config_error(true, "Codex CLI", "hook_uninstall", cp); + } + codex_toml_done: uninstall_agent_skill("Codex CLI", skills_dir, dry_run); uninstall_tiered_agent_profiles( (cbm_tiered_profile_set_t){ @@ -10179,15 +10170,10 @@ static void uninstall_cli_agents(const cbm_detected_agents_t *agents, const char }, dry_run); if (!dry_run) { - if (cbm_remove_codex_hooks(cp) != CLI_OK) { - record_agent_config_error(true, "Codex CLI", "hook_uninstall", cp); - } char hooks_json[CLI_BUF_1K]; - char hook_command[CLI_BUF_8K]; snprintf(hooks_json, sizeof(hooks_json), "%s/hooks.json", config_dir); if (cbm_file_exists(hooks_json) && - (cbm_build_augment_command(installed_binary, hook_command, sizeof(hook_command)) != - CLI_OK || + (!hook_command_ok || cbm_remove_paired_lifecycle_hooks_json(hooks_json, hook_command) != CLI_OK)) { record_agent_config_error(true, "Codex CLI", "json_hook_uninstall", hooks_json); } diff --git a/src/cli/config_toml_edit.c b/src/cli/config_toml_edit.c index 33a01d6cf..9b89dce47 100644 --- a/src/cli/config_toml_edit.c +++ b/src/cli/config_toml_edit.c @@ -1005,8 +1005,7 @@ int cbm_toml_upsert_managed_block(const char *file_path, const char *begin_marke return TOML_EDIT_ERR; } - toml_line_t begin_line = {0}; - toml_line_t end_line = {0}; + toml_line_t begin_line = {0}, end_line = {0}; int has_pair = 0; if (toml_find_markers(existing, existing_len, begin_marker, end_marker, &begin_line, &end_line, &has_pair) != TOML_EDIT_OK) { @@ -2600,3 +2599,429 @@ int cbm_toml_remove_legacy_table(const char *file_path, const char *table_name, free(existing); return result; } +typedef struct { + size_t start, end; +} toml_codex_edit_t; +static size_t toml_codex_payload_start(const char *data, size_t len) { + return len >= 3U && memcmp(data, "\xef\xbb\xbf", 3U) == 0 ? 3U : 0U; +} +static void toml_codex_skip_space(const char *data, size_t len, size_t *position) { + while (*position < len && (data[*position] == ' ' || data[*position] == '\t')) { + ++*position; + } +} +static int toml_codex_take(const char *data, size_t len, size_t *position, const char *expected) { + toml_codex_skip_space(data, len, position); + size_t expected_len = strlen(expected); + if (expected_len > len - *position || memcmp(data + *position, expected, expected_len) != 0) + return TOML_EDIT_ERR; + *position += expected_len; + return TOML_EDIT_OK; +} +static int toml_codex_take_string(const char *data, size_t len, size_t *position, + toml_string_t *value) { + toml_codex_skip_space(data, len, position); + if (*position >= len || + toml_parse_string(data + *position, len - *position, value) != TOML_EDIT_OK) + return TOML_EDIT_ERR; + *position += value->consumed; + return TOML_EDIT_OK; +} +static int toml_codex_take_field(const char *data, size_t len, size_t *position, const char *name) { + if (toml_codex_take(data, len, position, name) != TOML_EDIT_OK) + return TOML_EDIT_ERR; + return toml_codex_take(data, len, position, "="); +} +static int toml_codex_executable_is_safe(const char *encoded, size_t len, size_t *start, + size_t *end) { + if (!encoded || len == 0U) + return 0; + *start = encoded[0] == '\'' ? 1U : 0U; + *end = *start && len >= 2U && encoded[len - 1U] == '\'' ? len - 1U : len; + if (*start && *end == len) + return 0; + for (size_t pos = *start; pos < *end;) { + unsigned char ch = (unsigned char)encoded[pos++]; + if (ch < 0x21U || ch == 0x7fU || (!*start && strchr("\"'`$;&|<>(){}[]*?!", ch))) { + return 0; + } + if (*start && ch == '\'') { + if (pos < *end && encoded[pos] == '\'') { + ++pos; + } else if (pos + 2U < len && encoded[pos] == '\\' && encoded[pos + 1U] == '\'' && + encoded[pos + 2U] == '\'') { + pos += 3U; + } else { + return 0; + } + } + } + return *start < *end; +} +static int toml_codex_current_command_is_owned(const char *command, size_t len) { + static const char suffix[] = " hook-augment"; + size_t start = len >= 2U && command[0] == '&' && command[1] == ' ' ? 2U : 0U; + size_t suffix_len = sizeof(suffix) - 1U; + if (len <= start + suffix_len || memcmp(command + len - suffix_len, suffix, suffix_len) != 0) { + return 0; + } + const char *executable = command + start; + size_t executable_len = len - start - suffix_len; + size_t word_start = 0U; + size_t word_end = 0U; + if (!toml_codex_executable_is_safe(executable, executable_len, &word_start, &word_end)) { + return 0; + } + size_t basename = word_start; + for (size_t i = word_start; i < word_end; ++i) { + if (executable[i] == '/' || executable[i] == '\\') { + basename = i + 1U; + } + } + size_t basename_len = word_end - basename; + return (basename_len == strlen("codebase-memory-mcp") && + memcmp(executable + basename, "codebase-memory-mcp", basename_len) == 0) || + (basename_len == strlen("codebase-memory-mcp.exe") && + memcmp(executable + basename, "codebase-memory-mcp.exe", basename_len) == 0); +} +static int toml_codex_command_kind(const toml_string_t *command) { + static const char legacy_short[] = "echo \"Code discovery: prefer codebase-memory-mcp\""; + static const char legacy_released[] = + "echo \"Code discovery: prefer codebase-memory-mcp (search_graph, trace_path, " + "get_code_snippet, query_graph, search_code) over grep/file-read; run index_repository " + "first if the project is not indexed.\""; + if ((command->len == sizeof(legacy_short) - 1U && + memcmp(command->data, legacy_short, command->len) == 0) || + (command->len == sizeof(legacy_released) - 1U && + memcmp(command->data, legacy_released, command->len) == 0)) { + return 2; + } + return toml_codex_current_command_is_owned(command->data, command->len) ? 1 : 0; +} +static int toml_codex_inline_is_owned(const char *data, size_t start, size_t end, int event) { + size_t pos = start; + toml_string_t matcher = {0}; + toml_string_t type = {0}; + toml_string_t command = {0}; + toml_string_t windows = {0}; + const char *expected_matcher = event == 0 ? "startup|resume|clear|compact" : "*"; + int result = 0; + if (toml_codex_take(data, end, &pos, "[") != TOML_EDIT_OK || + toml_codex_take(data, end, &pos, "{") != TOML_EDIT_OK) + goto done; + if (toml_codex_take_field(data, end, &pos, "matcher") != TOML_EDIT_OK || + toml_codex_take_string(data, end, &pos, &matcher) != TOML_EDIT_OK) + goto done; + if (matcher.len != strlen(expected_matcher) || + memcmp(matcher.data, expected_matcher, matcher.len) != 0) + goto done; + if (toml_codex_take(data, end, &pos, ",") != TOML_EDIT_OK || + toml_codex_take_field(data, end, &pos, "hooks") != TOML_EDIT_OK || + toml_codex_take(data, end, &pos, "[") != TOML_EDIT_OK || + toml_codex_take(data, end, &pos, "{") != TOML_EDIT_OK) + goto done; + if (toml_codex_take_field(data, end, &pos, "type") != TOML_EDIT_OK || + toml_codex_take_string(data, end, &pos, &type) != TOML_EDIT_OK || type.len != 7U || + memcmp(type.data, "command", 7U) != 0) + goto done; + if (toml_codex_take(data, end, &pos, ",") != TOML_EDIT_OK || + toml_codex_take_field(data, end, &pos, "command") != TOML_EDIT_OK || + toml_codex_take_string(data, end, &pos, &command) != TOML_EDIT_OK) + goto done; + int command_kind = toml_codex_command_kind(&command); + if (command_kind == 1) { + if (toml_codex_take(data, end, &pos, ",") != TOML_EDIT_OK || + toml_codex_take_field(data, end, &pos, "command_windows") != TOML_EDIT_OK || + toml_codex_take_string(data, end, &pos, &windows) != TOML_EDIT_OK) + goto done; + if (!toml_codex_current_command_is_owned(windows.data, windows.len)) + goto done; + if (toml_codex_take(data, end, &pos, ",") != TOML_EDIT_OK || + toml_codex_take_field(data, end, &pos, "timeout") != TOML_EDIT_OK || + toml_codex_take(data, end, &pos, "5") != TOML_EDIT_OK) + goto done; + } else if (command_kind != 2 || event != 0) { + goto done; + } + if (toml_codex_take(data, end, &pos, "}") == TOML_EDIT_OK && + toml_codex_take(data, end, &pos, "]") == TOML_EDIT_OK && + toml_codex_take(data, end, &pos, "}") == TOML_EDIT_OK && + toml_codex_take(data, end, &pos, "]") == TOML_EDIT_OK) { + toml_codex_skip_space(data, end, &pos); + result = pos == end; + } +done: + toml_string_dispose(&matcher); + toml_string_dispose(&type); + toml_string_dispose(&command); + toml_string_dispose(&windows); + return result; +} +static int toml_codex_build_block(const char *command, const char *command_windows, + toml_buffer_t *block) { + char escaped[8192]; + char escaped_windows[8192]; + char rendered[24576]; + if (cbm_toml_escape_basic_string(command, escaped, sizeof(escaped)) != TOML_EDIT_OK || + cbm_toml_escape_basic_string(command_windows, escaped_windows, sizeof(escaped_windows)) != + TOML_EDIT_OK || + !toml_codex_current_command_is_owned(command, strlen(command)) || + !toml_codex_current_command_is_owned(command_windows, strlen(command_windows))) { + return TOML_EDIT_ERR; + } + int written = snprintf(rendered, sizeof(rendered), + "[[hooks.SessionStart]]\n" + "matcher = \"startup|resume|clear|compact\"\n\n" + "[[hooks.SessionStart.hooks]]\n" + "type = \"command\"\ncommand = \"%s\"\n" + "command_windows = \"%s\"\ntimeout = 5\n\n" + "[[hooks.SubagentStart]]\nmatcher = \"*\"\n\n" + "[[hooks.SubagentStart.hooks]]\n" + "type = \"command\"\ncommand = \"%s\"\n" + "command_windows = \"%s\"\ntimeout = 5\n", + escaped, escaped_windows, escaped, escaped_windows); + return written > 0 && (size_t)written < sizeof(rendered) + ? toml_buffer_append(block, rendered, (size_t)written) + : TOML_EDIT_ERR; +} +static size_t toml_codex_owned_span(const char *data, size_t len, const char *begin_marker, + const char *end_marker, const char *newline, int marked) { + toml_string_t values[2] = {{0}}; + size_t cursor = 0U; + toml_line_t line; + size_t result = 0U; + while ((!values[0].data || !values[1].data) && toml_next_line(data, len, &cursor, &line)) { + toml_assignment_t assignment; + if (toml_parse_assignment(data, &line, &assignment) != TOML_EDIT_OK) { + goto done; + } + int kind = toml_key_path_is_single(&assignment.key, "command") + ? 0 + : (toml_key_path_is_single(&assignment.key, "command_windows") ? 1 : -1); + if (assignment.present && kind >= 0 && !values[kind].data) { + size_t value_len = assignment.value_end - assignment.value_start; + if (assignment.multiline_value || + toml_parse_string(data + assignment.value_start, value_len, &values[kind]) != + TOML_EDIT_OK || + values[kind].consumed != value_len) { + toml_assignment_dispose(&assignment); + goto done; + } + } + toml_assignment_dispose(&assignment); + } + toml_buffer_t block = {0}; + toml_buffer_t expected = {0}; + if (toml_codex_current_command_is_owned(values[0].data, values[0].len) && + toml_codex_current_command_is_owned(values[1].data, values[1].len) && + toml_codex_build_block(values[0].data, values[1].data, &block) == TOML_EDIT_OK && + (marked ? toml_append_managed(&expected, begin_marker, end_marker, block.data, newline) + : toml_append_normalized_text(&expected, block.data, block.len, newline)) == + TOML_EDIT_OK && + expected.len <= len && memcmp(expected.data, data, expected.len) == 0) { + result = expected.len; + } + toml_buffer_dispose(&expected); + toml_buffer_dispose(&block); +done: + toml_string_dispose(&values[0]); + toml_string_dispose(&values[1]); + return result; +} +static int toml_codex_add_edit(toml_codex_edit_t *edits, size_t *count, size_t start, size_t end) { + if (*count >= 4U || start > end) + return TOML_EDIT_ERR; + edits[(*count)++] = (toml_codex_edit_t){.start = start, .end = end}; + return TOML_EDIT_OK; +} +static int toml_codex_scan_inline(const char *data, size_t len, toml_codex_edit_t *edits, + size_t *edit_count, int found[2]) { + toml_key_path_t wanted[2] = {{0}}; + toml_key_path_t scope = {0}; + if (toml_parse_key_path("hooks.SessionStart", 0U, strlen("hooks.SessionStart"), &wanted[0]) != + TOML_EDIT_OK || + toml_parse_key_path("hooks.SubagentStart", 0U, strlen("hooks.SubagentStart"), &wanted[1]) != + TOML_EDIT_OK) { + goto error; + } + size_t cursor = 0U; + toml_line_t line; + int multiline_state = TOML_STRING_NONE; + while (toml_next_line(data, len, &cursor, &line)) { + int in_multiline = multiline_state != TOML_STRING_NONE; + int header_present = 0; + if (!in_multiline) { + toml_header_t header; + if (toml_parse_header(data, &line, "", &header) != TOML_EDIT_OK) { + goto error; + } + header_present = header.present; + if (header.present) { + toml_key_path_dispose(&scope); + scope = header.path; + memset(&header.path, 0, sizeof(header.path)); + } + toml_header_dispose(&header); + } + if (!in_multiline && !header_present) { + toml_assignment_t assignment; + if (toml_parse_assignment(data, &line, &assignment) != TOML_EDIT_OK) { + goto error; + } + if (assignment.present) { + toml_key_path_t full_key; + if (toml_key_path_join(&scope, &assignment.key, &full_key) != TOML_EDIT_OK) { + toml_assignment_dispose(&assignment); + goto error; + } + int event = toml_key_path_equal(&full_key, &wanted[0]) + ? 0 + : (toml_key_path_equal(&full_key, &wanted[1]) ? 1 : -1); + toml_key_path_dispose(&full_key); + if (event >= 0) { + size_t tail = assignment.value_end; + toml_codex_skip_space(data, line.content_end, &tail); + size_t payload_start = toml_codex_payload_start(data, len); + size_t edit_start = line.start == 0U ? payload_start : line.start; + if (++found[event] > 1 || assignment.multiline_value || + tail != line.content_end || + !toml_codex_inline_is_owned(data, assignment.value_start, + assignment.value_end, event) || + toml_codex_add_edit(edits, edit_count, edit_start, line.full_end) != + TOML_EDIT_OK) { + toml_assignment_dispose(&assignment); + goto error; + } + } + toml_assignment_dispose(&assignment); + } + } + if (toml_scan_line_strings(data, &line, &multiline_state) != TOML_EDIT_OK) { + goto error; + } + } + int result = multiline_state == TOML_STRING_NONE ? TOML_EDIT_OK : TOML_EDIT_ERR; + goto done; +error: + result = TOML_EDIT_ERR; +done: + toml_key_path_dispose(&scope); + toml_key_path_dispose(&wanted[0]); + toml_key_path_dispose(&wanted[1]); + return result; +} +int cbm_toml_reconcile_codex_hooks(const char *file_path, const char *begin_marker, + const char *end_marker, const char *command, + const char *command_windows, cbm_toml_codex_hook_action_t action, + int check_only) { + if (!toml_valid_path(file_path) || !toml_valid_marker(begin_marker) || + !toml_valid_marker(end_marker) || strcmp(begin_marker, end_marker) == 0 || !command || + !command_windows || + (action != CBM_TOML_CODEX_HOOK_UPSERT && action != CBM_TOML_CODEX_HOOK_REMOVE)) { + return TOML_EDIT_ERR; + } + toml_buffer_t block = {0}; + if (toml_codex_build_block(command, command_windows, &block) != TOML_EDIT_OK) { + toml_buffer_dispose(&block); + return TOML_EDIT_ERR; + } + char *existing = NULL; + size_t existing_len = 0U; + toml_file_snapshot_t snapshot; + if (toml_read_file(file_path, &existing, &existing_len, &snapshot) != TOML_EDIT_OK || + !toml_text_is_safe(existing, existing_len, 1)) { + toml_buffer_dispose(&block); + free(existing); + return TOML_EDIT_ERR; + } + const char *newline = toml_newline_style(existing, existing_len); + toml_buffer_t output = {0}; + int result = TOML_EDIT_ERR; + toml_line_t begin_line = {0}; + toml_line_t end_line = {0}; + int has_pair = 0; + if (toml_find_markers(existing, existing_len, begin_marker, end_marker, &begin_line, &end_line, + &has_pair) != TOML_EDIT_OK) { + goto error; + } + size_t pair_start = begin_line.start; + if (has_pair && pair_start == 0U) { + pair_start = toml_codex_payload_start(existing, existing_len); + } + if (has_pair && + toml_codex_owned_span(existing + pair_start, end_line.full_end - pair_start, begin_marker, + end_marker, newline, 1) != end_line.full_end - pair_start) { + goto error; + } + toml_codex_edit_t edits[4]; + size_t edit_count = 0U; + if (has_pair && + toml_codex_add_edit(edits, &edit_count, pair_start, end_line.full_end) != TOML_EDIT_OK) { + goto error; + } + size_t payload_start = toml_codex_payload_start(existing, existing_len); + int markerless_count = 0; + static const char aot_prefix[] = "[[hooks.SessionStart]]"; + for (size_t pos = payload_start; sizeof(aot_prefix) - 1U <= existing_len - pos; ++pos) { + int line_start = pos == payload_start || existing[pos - 1U] == '\n'; + int in_pair = has_pair && pos >= pair_start && pos < end_line.full_end; + size_t owned_len = line_start && !in_pair && + memcmp(existing + pos, aot_prefix, sizeof(aot_prefix) - 1U) == 0 + ? toml_codex_owned_span(existing + pos, existing_len - pos, + begin_marker, end_marker, newline, 0) + : 0U; + if (owned_len) { + if (++markerless_count > 1 || + toml_codex_add_edit(edits, &edit_count, pos, pos + owned_len) != TOML_EDIT_OK) { + goto error; + } + pos += owned_len - 1U; + } + } + int inline_found[2] = {0}; + if (toml_codex_scan_inline(existing, existing_len, edits, &edit_count, inline_found) != + TOML_EDIT_OK || + (markerless_count && (has_pair || inline_found[0] || inline_found[1]))) { + goto error; + } + for (size_t i = 0U; i < edit_count; ++i) { + for (size_t j = i + 1U; j < edit_count; ++j) { + if (edits[j].start < edits[i].start) { + toml_codex_edit_t swap = edits[i]; + edits[i] = edits[j]; + edits[j] = swap; + } + } + if (i > 0U && edits[i - 1U].end > edits[i].start) { + goto error; + } + } + size_t cursor = 0U; + for (size_t i = 0U; i < edit_count; ++i) { + if (toml_buffer_append(&output, existing + cursor, edits[i].start - cursor) != + TOML_EDIT_OK) { + goto error; + } + cursor = edits[i].end; + } + if (toml_buffer_append(&output, existing + cursor, existing_len - cursor) != TOML_EDIT_OK) { + goto error; + } + if (action == CBM_TOML_CODEX_HOOK_UPSERT) { + size_t output_payload = toml_codex_payload_start(output.data, output.len); + if ((output.len > output_payload && output.data[output.len - 1U] != '\n' && + toml_buffer_append_cstr(&output, newline) != TOML_EDIT_OK) || + toml_append_managed(&output, begin_marker, end_marker, block.data, newline) != + TOML_EDIT_OK) { + goto error; + } + } + result = check_only ? TOML_EDIT_OK + : toml_write_atomic(file_path, existing, existing_len, output.data, + output.len, &snapshot); +error: + toml_buffer_dispose(&output); + toml_buffer_dispose(&block); + free(existing); + return result; +} diff --git a/src/cli/config_toml_edit.h b/src/cli/config_toml_edit.h index fcfde1207..ca73d4216 100644 --- a/src/cli/config_toml_edit.h +++ b/src/cli/config_toml_edit.h @@ -66,6 +66,14 @@ int cbm_toml_upsert_owned_named_array_table(const char *file_path, const char *t int cbm_toml_remove_owned_named_array_table(const char *file_path, const char *table_name, const char *identity_key, const char *identity_value, const char *canonical_body); +typedef enum { + CBM_TOML_CODEX_HOOK_UPSERT = 0, + CBM_TOML_CODEX_HOOK_REMOVE = 1 +} cbm_toml_codex_hook_action_t; +int cbm_toml_reconcile_codex_hooks(const char *file_path, const char *begin_marker, + const char *end_marker, const char *command, + const char *command_windows, cbm_toml_codex_hook_action_t action, + int check_only); #ifdef CBM_TOML_EDIT_ENABLE_TEST_API typedef void (*cbm_toml_precommit_test_hook_t)(const char *file_path, void *context); diff --git a/tests/test_cli.c b/tests/test_cli.c index 30bb3a337..ed1731f49 100644 --- a/tests/test_cli.c +++ b/tests/test_cli.c @@ -7970,6 +7970,22 @@ TEST(cli_codex_session_hook_issue330) { ASSERT_NULL(strstr(d, "hooks.SubagentStart")); ASSERT(strstr(d, "[mcp_servers.other]") != NULL); /* still preserved after removal */ + /* #1432: Codex may normalize the owned reminder to an inline assignment + * and discard our markers. Reinstall must replace that assignment instead + * of appending a duplicate TOML key. */ + write_test_file(cfg, "[hooks]\n" + "SessionStart = [{ matcher = \"startup|resume|clear|compact\", hooks = [{ " + "type = \"command\", command = \"echo \\\"Code discovery: prefer " + "codebase-memory-mcp\\\"\" }] }]\n\n" + "[mcp_servers.codebase-memory-mcp]\n" + "command = \"/Users/me/.local/bin/codebase-memory-mcp\"\n"); + ASSERT_EQ(cbm_upsert_codex_hooks(cfg), 0); + d = read_test_file(cfg); + ASSERT_NOT_NULL(d); + ASSERT_NULL(strstr(d, "SessionStart = [")); + ASSERT_NOT_NULL(strstr(d, "[[hooks.SessionStart]]")); + ASSERT_EQ(test_count_substring(d, "[[hooks.SessionStart]]"), 1U); + test_rmdir_r(tmpdir); PASS(); } @@ -8885,30 +8901,100 @@ TEST(cli_codex_migrates_to_single_hook_representation) { snprintf(codex_dir, sizeof(codex_dir), "%s/.codex", tmpdir); test_mkdirp(codex_dir); + char binary_dir[512]; + char binary_path[640]; + snprintf(binary_dir, sizeof(binary_dir), "%s/.local/bin", tmpdir); + test_mkdirp(binary_dir); +#ifdef _WIN32 + snprintf(binary_path, sizeof(binary_path), "%s/codebase-memory-mcp.exe", binary_dir); +#else + snprintf(binary_path, sizeof(binary_path), "%s/codebase-memory-mcp", binary_dir); +#endif + write_test_file(binary_path, "installed binary must survive failed cleanup\n"); + + char *saved_home = save_test_env("HOME"); char *saved_path = save_test_env("PATH"); char *saved_codex = save_test_env("CODEX_HOME"); + cbm_setenv("HOME", tmpdir, 1); cbm_setenv("PATH", tmpdir, 1); cbm_unsetenv("CODEX_HOME"); - cbm_install_agent_configs(tmpdir, "/opt/codebase-memory-mcp", false, false); char hooks_path[640]; char config_path[640]; snprintf(hooks_path, sizeof(hooks_path), "%s/hooks.json", codex_dir); snprintf(config_path, sizeof(config_path), "%s/config.toml", codex_dir); + int first_rc = cbm_install_agent_configs(tmpdir, binary_path, false, false); + char *first = read_test_file_alloc(config_path); + int repeat_rc = cbm_install_agent_configs(tmpdir, binary_path, false, false); + char *repeated = read_test_file_alloc(config_path); + int dry_rc = cbm_install_agent_configs(tmpdir, binary_path, false, true); + char *after_dry = read_test_file_alloc(config_path); + + write_test_file(config_path, + "[hooks]\nSessionStart = [{ matcher = \"startup|resume|clear|compact\", " + "hooks = [{ type = \"command\", command = \"echo \\\"Code discovery: " + "prefer codebase-memory-mcp\\\"\" }] }]\n"); write_test_file(hooks_path, "{}\n"); - cbm_install_agent_configs(tmpdir, "/opt/codebase-memory-mcp", false, false); + int migration_rc = cbm_install_agent_configs(tmpdir, binary_path, false, false); char *toml = read_test_file_alloc(config_path); char *hooks = read_test_file_alloc(hooks_path); - bool migrated = toml && !strstr(toml, "codebase-memory-mcp SessionStart") && hooks && + bool lifecycle_ok = first_rc == 0 && repeat_rc == 0 && dry_rc == 0 && first && repeated && + after_dry && strcmp(first, repeated) == 0 && strcmp(first, after_dry) == 0; + bool migrated = migration_rc == 0 && toml && !strstr(toml, "SessionStart") && hooks && strstr(hooks, "SessionStart") && strstr(hooks, "SubagentStart"); + free(first); + free(repeated); + free(after_dry); free(toml); free(hooks); + + const char *ambiguous = + "[hooks]\nSessionStart = [{ matcher = \"startup|resume|clear|compact\", hooks = [" + "{ type = \"command\", command = 'echo \"Code discovery: prefer " + "codebase-memory-mcp\"' }, { type = \"command\", command = \"foreign\" }] }]\n"; + write_test_file(config_path, ambiguous); + char *uninstall_argv[] = {"--yes"}; + int uninstall_rc = cli_test_cmd_uninstall(1, uninstall_argv); + char skill_path[768]; + char agent_path[768]; + snprintf(skill_path, sizeof(skill_path), "%s/skills/codebase-memory/SKILL.md", codex_dir); + snprintf(agent_path, sizeof(agent_path), "%s/agents/codebase-memory.toml", codex_dir); + struct stat state; + hooks = read_test_file_alloc(hooks_path); + bool independent_cleanup = uninstall_rc != 0 && stat(binary_path, &state) == 0 && + stat(skill_path, &state) != 0 && stat(agent_path, &state) != 0 && + hooks && !strstr(hooks, "hook-augment"); + free(hooks); + + char bad_home[256]; + snprintf(bad_home, sizeof(bad_home), "/tmp/cli-codex-preflight-XXXXXX"); + bool no_partial = false; + if (cbm_mkdtemp(bad_home)) { + char bad_codex[512]; + char bad_config[640]; + char bad_agents[640]; + snprintf(bad_codex, sizeof(bad_codex), "%s/.codex", bad_home); + snprintf(bad_config, sizeof(bad_config), "%s/config.toml", bad_codex); + snprintf(bad_agents, sizeof(bad_agents), "%s/AGENTS.md", bad_codex); + test_mkdirp(bad_codex); + write_test_file(bad_config, ambiguous); + cbm_setenv("HOME", bad_home, 1); + cbm_setenv("PATH", bad_home, 1); + int bad_rc = cbm_install_agent_configs(bad_home, binary_path, false, false); + char *bad_after = read_test_file_alloc(bad_config); + no_partial = bad_rc != 0 && bad_after && strcmp(bad_after, ambiguous) == 0 && + stat(bad_agents, &state) != 0; + free(bad_after); + test_rmdir_r(bad_home); + } + restore_test_env("HOME", saved_home); restore_test_env("PATH", saved_path); restore_test_env("CODEX_HOME", saved_codex); test_rmdir_r(tmpdir); - if (!migrated) - FAIL("Codex install must leave exactly one lifecycle hook representation"); + if (!lifecycle_ok || !migrated || !independent_cleanup || !no_partial) + FAIL("Codex lifecycle preflight must be idempotent, transactional, and independently " + "clean owned side files"); PASS(); } diff --git a/tests/test_config_toml_edit.c b/tests/test_config_toml_edit.c index 90fa504ef..0c14e6bf1 100644 --- a/tests/test_config_toml_edit.c +++ b/tests/test_config_toml_edit.c @@ -31,6 +31,24 @@ static const char *CTE_KEY = "name"; static const char *CTE_IDENTITY = "codebase-memory-mcp"; static const char *CTE_BODY = "name = \"codebase-memory-mcp\"\n" "command = \"codebase-memory-mcp\"\n"; +static const char *CTE_CODEX_BEGIN = "# >>> codebase-memory-mcp SessionStart >>>"; +static const char *CTE_CODEX_END = "# <<< codebase-memory-mcp SessionStart <<<"; +static const char *CTE_CODEX_COMMAND = "codebase-memory-mcp hook-augment"; +static const char *CTE_CODEX_BLOCK = + "[[hooks.SessionStart]]\n" + "matcher = \"startup|resume|clear|compact\"\n\n" + "[[hooks.SessionStart.hooks]]\n" + "type = \"command\"\ncommand = \"/opt/codebase-memory-mcp hook-augment\"\n" + "command_windows = \"& C:\\\\bin\\\\codebase-memory-mcp.exe hook-augment\"\ntimeout = 5\n\n" + "[[hooks.SubagentStart]]\nmatcher = \"*\"\n\n" + "[[hooks.SubagentStart.hooks]]\n" + "type = \"command\"\ncommand = \"/opt/codebase-memory-mcp hook-augment\"\n" + "command_windows = \"& C:\\\\bin\\\\codebase-memory-mcp.exe hook-augment\"\ntimeout = 5\n"; + +static int cte_codex_edit(const char *path, cbm_toml_codex_hook_action_t action, int check_only) { + return cbm_toml_reconcile_codex_hooks(path, CTE_CODEX_BEGIN, CTE_CODEX_END, CTE_CODEX_COMMAND, + CTE_CODEX_COMMAND, action, check_only); +} static int cte_fixture(char *dir, size_t dir_size, char *path, size_t path_size) { char *created = th_mktempdir("cbm_toml_edit"); @@ -1022,6 +1040,122 @@ TEST(config_toml_vibe_ambiguous_target_fail_closed) { PASS(); } +TEST(config_toml_codex_reconciles_minimal_owned_forms) { + char dir[CTE_PATH_CAP]; + char path[CTE_PATH_CAP]; + char actual[CTE_FILE_CAP]; + char before[CTE_FILE_CAP]; + ASSERT_EQ(cte_fixture(dir, sizeof(dir), path, sizeof(path)), 0); + ASSERT_EQ(cte_codex_edit(path, CBM_TOML_CODEX_HOOK_UPSERT, 1), 0); + ASSERT_EQ(cte_read(path, actual, sizeof(actual)), -1); + ASSERT_EQ(cte_codex_edit(path, CBM_TOML_CODEX_HOOK_UPSERT, 0), 0); + ASSERT_EQ(cte_read(path, actual, sizeof(actual)), 0); + ASSERT_NOT_NULL(strstr(actual, CTE_CODEX_BEGIN)); + ASSERT_EQ(cte_codex_edit(path, CBM_TOML_CODEX_HOOK_REMOVE, 0), 0); + + static const char *fixtures[] = { + "[hooks]\nSessionStart = [{ matcher = \"startup|resume|clear|compact\", hooks = " + "[{ type = \"command\", command = 'echo \"Code discovery: prefer " + "codebase-memory-mcp\"' }] }]\n", + "\"hooks\" . 'SessionStart' = [ {matcher='startup|resume|clear|compact', " + "hooks=[{type='command', command='/opt/codebase-memory-mcp hook-augment', " + "command_windows='& C:\\\\bin\\\\codebase-memory-mcp.exe hook-augment',timeout=5}]} ]\n", + }; + for (size_t i = 0U; i < sizeof(fixtures) / sizeof(fixtures[0]); ++i) { + ASSERT_EQ(th_write_file(path, fixtures[i]), 0); + ASSERT_EQ(cte_read(path, before, sizeof(before)), 0); + ASSERT_EQ(cte_codex_edit(path, CBM_TOML_CODEX_HOOK_UPSERT, 1), 0); + ASSERT_EQ(cte_read(path, actual, sizeof(actual)), 0); + ASSERT_STR_EQ(actual, before); + ASSERT_EQ(cte_codex_edit(path, CBM_TOML_CODEX_HOOK_UPSERT, 0), 0); + ASSERT_EQ(cte_read(path, actual, sizeof(actual)), 0); + ASSERT_NULL(strstr(actual, "SessionStart = [")); + ASSERT_EQ(cte_occurrences(actual, CTE_CODEX_BEGIN), 1); + ASSERT_EQ(cte_read(path, before, sizeof(before)), 0); + ASSERT_EQ(cte_codex_edit(path, CBM_TOML_CODEX_HOOK_UPSERT, 0), 0); + ASSERT_EQ(cte_read(path, actual, sizeof(actual)), 0); + ASSERT_STR_EQ(actual, before); + ASSERT_EQ(cte_codex_edit(path, CBM_TOML_CODEX_HOOK_REMOVE, 0), 0); + ASSERT_EQ(cte_read(path, actual, sizeof(actual)), 0); + ASSERT_NULL(strstr(actual, "hook-augment")); + } + + ASSERT_EQ(th_write_file(path, CTE_CODEX_BLOCK), 0); + ASSERT_EQ(cte_codex_edit(path, CBM_TOML_CODEX_HOOK_UPSERT, 0), 0); + ASSERT_EQ(cte_read(path, actual, sizeof(actual)), 0); + ASSERT_EQ(cte_occurrences(actual, CTE_CODEX_BEGIN), 1); + ASSERT_EQ(cte_occurrences(actual, "[[hooks.SessionStart]]"), 1); + char damaged[CTE_FILE_CAP]; + ASSERT_GT(snprintf(damaged, sizeof(damaged), "%s%s\n%s%s\n%s\n", fixtures[0], CTE_CODEX_BEGIN, + CTE_CODEX_BLOCK, CTE_CODEX_END, "keep = true"), + 0); + ASSERT_EQ(th_write_file(path, damaged), 0); + ASSERT_EQ(cte_codex_edit(path, CBM_TOML_CODEX_HOOK_UPSERT, 0), 0); + ASSERT_EQ(cte_read(path, actual, sizeof(actual)), 0); + ASSERT_NULL(strstr(actual, "SessionStart = [")); + ASSERT_EQ(cte_occurrences(actual, CTE_CODEX_BEGIN), 1); + ASSERT_NOT_NULL(strstr(actual, "keep = true")); + th_cleanup(dir); + PASS(); +} + +TEST(config_toml_codex_rejects_ambiguous_inline_byte_identically) { + char dir[CTE_PATH_CAP]; + char path[CTE_PATH_CAP]; + char actual[CTE_FILE_CAP]; + static const char *invalid[] = { + ("[hooks]\nSessionStart = [{matcher='startup|resume|clear|compact',hooks=[{type=" + "'command',command='codebase-memory-mcp hook-augment',command_windows=" + "'codebase-memory-mcp hook-augment',timeout=5},{type='command',command='foreign'}]}]\n"), + ("[hooks]\nSessionStart = [{matcher='startup|resume|clear|compact',hooks=[{type=" + "'command',command='codebase-memory-mcp hook-augment',command_windows=" + "'codebase-memory-mcp hook-augment',timeout=5,owner='user'}]}]\n"), + "[hooks]\nSessionStart=[]\nSessionStart=[]\n", + "[hooks]\nSessionStart = [\n { matcher = 'startup|resume|clear|compact' }\n]\n", + ("[hooks]\nSessionStart = [{matcher='startup|resume|clear|compact',hooks=[{type=" + "'command',command='echo \"Code discovery: prefer codebase-memory-mcp\"'}]}] # keep\n"), + "[hooks]\nSessionStart = [{ matcher = 'startup|resume|clear|compact'\n", + }; + ASSERT_EQ(cte_fixture(dir, sizeof(dir), path, sizeof(path)), 0); + for (size_t i = 0U; i < sizeof(invalid) / sizeof(invalid[0]); ++i) { + ASSERT_EQ(th_write_file(path, invalid[i]), 0); + ASSERT_EQ(cte_codex_edit(path, CBM_TOML_CODEX_HOOK_UPSERT, 1), -1); + ASSERT_EQ(cte_codex_edit(path, CBM_TOML_CODEX_HOOK_UPSERT, 0), -1); + ASSERT_EQ(cte_codex_edit(path, CBM_TOML_CODEX_HOOK_REMOVE, 0), -1); + ASSERT_EQ(cte_read(path, actual, sizeof(actual)), 0); + ASSERT_STR_EQ(actual, invalid[i]); + } + th_cleanup(dir); + PASS(); +} + +TEST(config_toml_codex_preserves_bom_crlf_and_foreign_aot) { + char dir[CTE_PATH_CAP]; + char path[CTE_PATH_CAP]; + char actual[CTE_FILE_CAP]; + const char *foreign = "\xEF\xBB\xBF[[hooks.SessionStart]]\r\nmatcher = \"custom\"\r\n" + "[[hooks.SessionStart.hooks]]\r\ntype = \"command\"\r\n" + "command = \"foreign\"\r\ntimeout = 9\r\n"; + ASSERT_EQ(cte_fixture(dir, sizeof(dir), path, sizeof(path)), 0); + ASSERT_EQ(th_write_file(path, foreign), 0); + ASSERT_EQ(cte_codex_edit(path, CBM_TOML_CODEX_HOOK_UPSERT, 0), 0); + ASSERT_EQ(cte_read(path, actual, sizeof(actual)), 0); + ASSERT_EQ((unsigned char)actual[0], 0xEFU); + ASSERT_NOT_NULL(strstr(actual, "command = \"foreign\"")); + ASSERT_NOT_NULL(strstr(actual, "\r\n# >>> codebase-memory-mcp SessionStart >>>\r\n")); + for (const char *cursor = actual; *cursor; ++cursor) { + if (*cursor == '\n') { + ASSERT(cursor > actual && cursor[-1] == '\r'); + } + } + ASSERT_EQ(cte_codex_edit(path, CBM_TOML_CODEX_HOOK_REMOVE, 0), 0); + ASSERT_EQ(cte_read(path, actual, sizeof(actual)), 0); + ASSERT_NOT_NULL(strstr(actual, "command = \"foreign\"")); + ASSERT_NULL(strstr(actual, CTE_CODEX_COMMAND)); + th_cleanup(dir); + PASS(); +} + SUITE(config_toml_edit) { RUN_TEST(config_toml_rejects_stale_content_and_identity); RUN_TEST(config_toml_missing_target_race_does_not_replace_winner); @@ -1058,5 +1192,8 @@ SUITE(config_toml_edit) { RUN_TEST(config_toml_vibe_duplicate_target_fail_closed); RUN_TEST(config_toml_vibe_ambiguous_target_fail_closed); RUN_TEST(config_toml_target_table_rejects_significant_nonassignments_byte_identically); + RUN_TEST(config_toml_codex_reconciles_minimal_owned_forms); + RUN_TEST(config_toml_codex_rejects_ambiguous_inline_byte_identically); + RUN_TEST(config_toml_codex_preserves_bom_crlf_and_foreign_aot); RUN_TEST(config_toml_legacy_remove_reports_foreign_table_without_mutation); }