From 95f252447de4ae08707b199f02f296f28f8dcb21 Mon Sep 17 00:00:00 2001 From: Joshua Richter Date: Tue, 8 Sep 2026 00:41:23 -0400 Subject: [PATCH] fix: uninstall removes the executable and indexes even when an agent config cleanup fails One agent configuration that could not be cleaned up stopped the whole uninstall before index and executable removal started. The reporter had ~/.cursor/mcp.json symlinked into a dotfiles repository; the JSON editors open with O_NOFOLLOW and refuse to write through the link, and that one refusal left a 282 MB executable and a 104 MB index cache behind (#1954). Cleaning up an agent's configuration and removing this tool's own files are separate jobs, so the early return is gone. Every failure is still named on stderr as it happens, is counted, and still makes the exit code non-zero. The closing line now states the count, because a reader who watches stdout alone never sees the stderr lines. CBM_AGENT_EDIT_NOT_APPLICABLE also stopped counting as a failure in the agent client registry loop. That value means the client has no MCP editor for its config shape, so there was never an entry of ours to take out. The per-client report also said "removed" whatever happened: --dry-run said "removed" for files it never opened, a step that had just recorded an error said "removed" on the next line, and a preserved foreign entry printed "preserved" and "removed" one after the other. Two helpers fix that. uninstall_verb(dry_run) returns "would remove" or "removed". uninstall_steps_succeeded(errors_before) ties a line covering several steps to whether every one of those steps worked. A failed step now prints no line of its own. The shape copies the Qoder and Devin blocks, which already reported planned / complete / failed correctly. Two tests are new. Two existing tests encoded the old contract and are inverted: cli_agent_uninstall_reports_safe_editor_refusal and cli_codex_migrates_to_single_hook_representation now assert the executable is gone after a failed config cleanup. Both still assert the non-zero exit and the untouched config file. Refs #1954 Signed-off-by: Joshua Richter --- src/cli/cli.c | 182 ++++++++++++++++++++++++++++++--------- tests/test_cli.c | 215 +++++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 351 insertions(+), 46 deletions(-) diff --git a/src/cli/cli.c b/src/cli/cli.c index d85ebf0b6..a224310e7 100644 --- a/src/cli/cli.c +++ b/src/cli/cli.c @@ -10802,6 +10802,30 @@ int cbm_cmd_install(int argc, char **argv) { /* ── Subcommand: uninstall ────────────────────────────────────── */ +/* One wording for every line the uninstall report prints. + * + * The report used to be written in the past tense whatever happened: a + * --dry-run said "removed" for files it never opened, and a step that had just + * recorded an error said "removed" on the next line (#1954). A reader could + * only conclude that a dozen configuration files had been rewritten. + * + * Every report line now goes through this verb, and the caller prints the line + * only when the step really ran and worked. A failed step prints nothing here + * because record_agent_config_error has already named the agent, the operation + * and the path on stderr. The shape copies the Qoder and Devin blocks below, + * which already said "planned" for a dry run and "failed" for a failure. */ +static const char *uninstall_verb(bool dry_run) { + return dry_run ? "would remove" : "removed"; +} + +/* True when nothing has recorded a failure since `errors_before` was read from + * g_agent_uninstall_errors. Several report lines below cover a run of steps at + * once ("removed MCP config + hooks + instructions"), and this ties such a line + * to whether every one of those steps worked. */ +static bool uninstall_steps_succeeded(int errors_before) { + return g_agent_uninstall_errors == errors_before; +} + /* Remove Claude Code agent configs. */ static void uninstall_claude_code(const char *home, const char *installed_binary, bool dry_run) { char config_dir[CLI_BUF_1K]; @@ -10812,7 +10836,7 @@ static void uninstall_claude_code(const char *home, const char *installed_binary char skills_dir[CLI_BUF_1K]; snprintf(skills_dir, sizeof(skills_dir), "%s/skills", config_dir); int removed = cbm_remove_skills(skills_dir, dry_run); - printf("Claude Code: removed %d skill(s)\n", removed); + printf("Claude Code: %s %d skill(s)\n", uninstall_verb(dry_run), removed); char agent_path[CLI_BUF_1K]; snprintf(agent_path, sizeof(agent_path), "%s/agents/codebase-memory.md", config_dir); uninstall_tiered_agent_profiles( @@ -10827,10 +10851,14 @@ static void uninstall_claude_code(const char *home, const char *installed_binary char mcp_path[CLI_BUF_1K]; snprintf(mcp_path, sizeof(mcp_path), "%s/.mcp.json", config_dir); + bool legacy_mcp_removed = true; if (!dry_run && cbm_remove_editor_mcp_owned(installed_binary, mcp_path) != CLI_OK) { record_agent_config_error(true, "Claude Code", "legacy_mcp_uninstall", mcp_path); + legacy_mcp_removed = false; + } + if (legacy_mcp_removed) { + printf(" %s MCP config entry\n", uninstall_verb(dry_run)); } - printf(" removed MCP config entry\n"); char mcp_path2[CLI_BUF_1K]; snprintf(mcp_path2, sizeof(mcp_path2), "%s/.claude.json", user_root); @@ -10840,6 +10868,7 @@ static void uninstall_claude_code(const char *home, const char *installed_binary char settings_path[CLI_BUF_1K]; snprintf(settings_path, sizeof(settings_path), "%s/settings.json", config_dir); + int hook_errors_before = g_agent_uninstall_errors; if (!dry_run) { if (cbm_remove_claude_hooks_with_binary(settings_path, installed_binary) != CLI_OK) { record_agent_config_error(true, "Claude Code", "pretool_hook_uninstall", settings_path); @@ -10933,7 +10962,9 @@ static void uninstall_claude_code(const char *home, const char *installed_binary #endif } } - printf(" removed PreToolUse + SessionStart + SubagentStart hooks\n"); + if (uninstall_steps_succeeded(hook_errors_before)) { + printf(" %s PreToolUse + SessionStart + SubagentStart hooks\n", uninstall_verb(dry_run)); + } } /* Remove MCP + instructions for a generic agent. */ @@ -10943,28 +10974,41 @@ typedef struct { const char *config_path; const char *instr_path; } mcp_uninstall_args_t; +/* Remove MCP + instructions for a generic agent, and report only what the run + * really did. Both lines below used to print every time: a config entry that + * was preserved because it had been modified said "preserved" and "removed" one + * after the other, a config the run had just failed to edit still said + * "removed", and a --dry-run said "removed" for files it never opened + * (#1954). */ static void uninstall_agent_mcp_instr(mcp_uninstall_args_t paths, bool dry_run, int (*remove_fn)(const char *, const char *)) { const char *name = paths.name; const char *instr_path = paths.instr_path; + bool mcp_removed = true; if (!dry_run) { char binary_path[CLI_BUF_1K]; cbm_agent_installed_binary_path(cbm_get_home_dir(), binary_path, sizeof(binary_path)); int remove_result = remove_fn(binary_path, paths.config_path); if (remove_result < CLI_OK) { record_agent_config_error(true, name, "mcp_uninstall", paths.config_path); + mcp_removed = false; } else if (remove_result > CLI_OK) { printf("%s: preserved modified or foreign MCP entry\n", name); + mcp_removed = false; } } - printf("%s: removed MCP config entry\n", name); + if (mcp_removed) { + printf("%s: %s MCP config entry\n", name, uninstall_verb(dry_run)); + } if (instr_path) { - if (!dry_run) { - if (cbm_remove_instructions(instr_path) != CLI_OK) { - record_agent_config_error(true, name, "instructions_uninstall", instr_path); - } + bool instructions_removed = true; + if (!dry_run && cbm_remove_instructions(instr_path) != CLI_OK) { + record_agent_config_error(true, name, "instructions_uninstall", instr_path); + instructions_removed = false; + } + if (instructions_removed) { + printf(" %s instructions\n", uninstall_verb(dry_run)); } - printf(" removed instructions\n"); } } @@ -10972,16 +11016,18 @@ static bool uninstall_codex_activation_pointer(const char *path, bool dry_run) { return path && (dry_run || cbm_remove_instructions(path) == CLI_OK); } -static void report_codex_activation_pointer_uninstall(const char *path, bool removed) { - printf(" instructions: removed managed activation pointer\n"); - if (!removed) { +static void report_codex_activation_pointer_uninstall(const char *path, bool removed, + bool dry_run) { + if (removed) { + printf(" instructions: %s managed activation pointer\n", uninstall_verb(dry_run)); + } else { record_agent_config_error(true, "Codex CLI", "instructions_uninstall", path); } } static void uninstall_agent_skill(const char *label, const char *skills_dir, bool dry_run) { int removed = cbm_remove_skills(skills_dir, dry_run); - printf(" %s skill: %d removed\n", label, removed); + printf(" %s skill: %d %s\n", label, removed, dry_run ? "would be removed" : "removed"); } static void uninstall_copilot_durable_context(const char *home, bool dry_run) { @@ -10995,8 +11041,10 @@ static void uninstall_copilot_durable_context(const char *home, bool dry_run) { snprintf(skills_dir, sizeof(skills_dir), "%s/skills", config_dir); snprintf(agent_path, sizeof(agent_path), "%s/agents/codebase-memory.agent.md", config_dir); cbm_agent_installed_binary_path(home, binary_path, sizeof(binary_path)); + bool copilot_hooks_removed = true; if (!dry_run && cbm_remove_copilot_hooks(hook_path, binary_path) != CLI_OK) { record_agent_config_error(true, "Copilot", "lifecycle_hook_uninstall", hook_path); + copilot_hooks_removed = false; } uninstall_agent_skill("Copilot", skills_dir, dry_run); uninstall_tiered_agent_profiles( @@ -11008,7 +11056,9 @@ static void uninstall_copilot_durable_context(const char *home, bool dry_run) { .dialect = CBM_GRAPH_DIALECT_COPILOT, }, dry_run); - printf(" removed SessionStart + SubagentStart hooks\n"); + if (copilot_hooks_removed) { + printf(" %s SessionStart + SubagentStart hooks\n", uninstall_verb(dry_run)); + } } static int cbm_remove_managed_instructions(const char *instructions_path) { @@ -11072,8 +11122,9 @@ static void uninstall_gitlab_durable_context(const cbm_agent_registry_context_t } if (!dry_run && cbm_remove_gitlab_session_hook(hooks_path, binary_path) != CLI_OK) { record_agent_config_error(true, "GitLab Duo CLI", "session_hook_uninstall", hooks_path); + } else { + printf(" hook: %s canonical SessionStart entry\n", uninstall_verb(dry_run)); } - printf(" hook: removed canonical SessionStart entry\n"); } static void uninstall_devin_durable_context(const cbm_agent_registry_context_t *registry, @@ -11113,8 +11164,9 @@ static void uninstall_pi_durable_context(const char *home, bool dry_run) { snprintf(skills_dir, sizeof(skills_dir), "%s/.pi/agent/skills", home); if (!dry_run && cbm_remove_managed_instructions(instructions_path) != CLI_OK) { record_agent_config_error(true, "Pi", "instructions_uninstall", instructions_path); + } else { + printf(" instructions: %s managed context\n", uninstall_verb(dry_run)); } - printf(" instructions: removed managed context\n"); uninstall_agent_skill("Pi", skills_dir, dry_run); char extension_path[CLI_BUF_1K]; snprintf(extension_path, sizeof(extension_path), "%s/.pi/agent/extensions/cbmem.ts", home); @@ -11125,8 +11177,9 @@ static void uninstall_managed_agent_instructions(const char *label, const char * bool dry_run) { if (!dry_run && cbm_remove_managed_instructions(instructions_path) != CLI_OK) { record_agent_config_error(true, label, "instructions_uninstall", instructions_path); + } else { + printf(" instructions: %s managed context\n", uninstall_verb(dry_run)); } - printf(" instructions: removed managed context\n"); } static bool remove_cline_context_hooks(const char *cline_root, const char *binary_path, @@ -11178,8 +11231,9 @@ static void uninstall_kimi_durable_context(const cbm_agent_registry_context_t *r snprintf(config_path, sizeof(config_path), "%s/config.toml", kimi_home); if (!dry_run && cbm_remove_kimi_context_hook(config_path) != CLI_OK) { record_agent_config_error(true, "Kimi Code CLI", "prompt_hook_uninstall", config_path); + } else { + printf(" hook: %s managed UserPromptSubmit entry\n", uninstall_verb(dry_run)); } - printf(" hook: removed managed UserPromptSubmit entry\n"); uninstall_managed_agent_instructions("Kimi Code CLI", instructions_path, dry_run); uninstall_agent_skill("Kimi Code CLI", skills_dir, dry_run); } @@ -11318,11 +11372,18 @@ static void uninstall_agent_client_registry(const char *home, bool dry_run) { : profile->remove_mcp(profile->id, config_path, binary_path); if (edit_result == CBM_AGENT_EDIT_FOREIGN) { printf(" mcp: preserved modified or foreign entry in %s\n", config_path); + } else if (edit_result == CBM_AGENT_EDIT_NOT_APPLICABLE) { + /* The client has no MCP editor for this config shape, so + * there was never an entry of ours to take out. Counting + * that as a failure is what made a whole uninstall refuse + * over a file it had never written to (#1954). */ + printf(" mcp: no managed entry to remove in %s\n", config_path); } else if (edit_result != CBM_AGENT_EDIT_OK) { record_agent_config_error(true, profile->display_name, "mcp_uninstall", config_path); } else { - printf(" mcp: removed canonical entry from %s\n", config_path); + printf(" mcp: %s canonical entry from %s\n", uninstall_verb(dry_run), + config_path); } } } @@ -11368,6 +11429,7 @@ static void uninstall_gemini_config(const char *home, bool dry_run) { snprintf(cp, sizeof(cp), "%s/.gemini/settings.json", home); snprintf(ip, sizeof(ip), "%s/.gemini/GEMINI.md", home); snprintf(ap, sizeof(ap), "%s/.gemini/agents/codebase-memory.md", home); + int gemini_errors_before = g_agent_uninstall_errors; if (!dry_run) { if (cbm_remove_editor_mcp_owned(installed_binary, cp) != CLI_OK) { record_agent_config_error(true, "Gemini CLI", "mcp_uninstall", cp); @@ -11396,7 +11458,10 @@ static void uninstall_gemini_config(const char *home, bool dry_run) { .dialect = CBM_GRAPH_DIALECT_GEMINI, }, dry_run); - printf("Gemini CLI: removed MCP config + hooks + instructions + tiered subagents\n"); + if (uninstall_steps_succeeded(gemini_errors_before)) { + printf("Gemini CLI: %s MCP config + hooks + instructions + tiered subagents\n", + uninstall_verb(dry_run)); + } } static void uninstall_cli_agents(const cbm_detected_agents_t *agents, const char *home, @@ -11434,12 +11499,12 @@ static void uninstall_cli_agents(const cbm_detected_agents_t *agents, const char ? NULL : cbm_toml_codex_hook_failure_name(preflight_failure); record_agent_config_error_with_reason(true, "Codex CLI", "hook_preflight", cp, reason); - report_codex_activation_pointer_uninstall(ip, pointer_removed); + report_codex_activation_pointer_uninstall(ip, pointer_removed, dry_run); goto codex_toml_done; } uninstall_agent_mcp_instr((mcp_uninstall_args_t){"Codex CLI", cp, NULL}, dry_run, cbm_remove_codex_mcp_owned); - report_codex_activation_pointer_uninstall(ip, pointer_removed); + report_codex_activation_pointer_uninstall(ip, pointer_removed, dry_run); if (!dry_run && cbm_reconcile_codex_hooks_command(cp, hook_command, hook_command_windows, CBM_TOML_CODEX_HOOK_REMOVE, false) != CLI_OK) { @@ -11514,6 +11579,7 @@ static void uninstall_cli_agents(const cbm_detected_agents_t *agents, const char char ip[CLI_BUF_1K]; snprintf(cp, sizeof(cp), "%s/.aider.conf.yml", home); snprintf(ip, sizeof(ip), "%s/CONVENTIONS.md", home); + int aider_errors_before = g_agent_uninstall_errors; if (!dry_run) { if (cbm_yaml_remove_string_list_item(cp, "read", ip) != CLI_OK) { record_agent_config_error(true, "Aider", "loader_uninstall", cp); @@ -11522,7 +11588,9 @@ static void uninstall_cli_agents(const cbm_detected_agents_t *agents, const char record_agent_config_error(true, "Aider", "instructions_uninstall", ip); } } - printf("Aider: removed instructions + loader reference\n"); + if (uninstall_steps_succeeded(aider_errors_before)) { + printf("Aider: %s instructions + loader reference\n", uninstall_verb(dry_run)); + } } } @@ -11551,6 +11619,7 @@ static void uninstall_editor_agents(const cbm_detected_agents_t *agents, const c snprintf(cp, sizeof(cp), "%s/.config/kilo/kilo.jsonc", home); snprintf(ip, sizeof(ip), "%s/.config/kilo/rules/codebase-memory-mcp.md", home); snprintf(ap, sizeof(ap), "%s/.config/kilo/agents/codebase-memory.md", home); + int kilo_errors_before = g_agent_uninstall_errors; if (!dry_run) { if (cbm_remove_kilo_mcp_owned(installed_binary, cp) != CLI_OK) { record_agent_config_error(true, "KiloCode", "mcp_uninstall", cp); @@ -11598,7 +11667,9 @@ static void uninstall_editor_agents(const cbm_detected_agents_t *agents, const c .dialect = CBM_GRAPH_DIALECT_KILO, }, dry_run); - printf("KiloCode: removed MCP config + instruction reference\n"); + if (uninstall_steps_succeeded(kilo_errors_before)) { + printf("KiloCode: %s MCP config + instruction reference\n", uninstall_verb(dry_run)); + } } if (agents->vscode) { char code_user[CLI_BUF_1K]; @@ -11647,6 +11718,7 @@ static void uninstall_editor_agents(const cbm_detected_agents_t *agents, const c bool workspace_ok = cbm_openclaw_workspace_path(home, cp, workspace, sizeof(workspace)); uninstall_agent_mcp_instr((mcp_uninstall_args_t){"OpenClaw", cp, NULL}, dry_run, cbm_remove_openclaw_mcp_owned); + int openclaw_errors_before = g_agent_uninstall_errors; if (!dry_run && cbm_remove_openclaw_compaction(cp) != CLI_OK) { record_agent_config_error(true, "OpenClaw", "compaction_uninstall", cp); } @@ -11665,9 +11737,13 @@ static void uninstall_editor_agents(const cbm_detected_agents_t *agents, const c tools_path); } } - printf(" removed workspace instructions + compaction augmentation\n"); - } else { - printf(" removed compaction augmentation; workspace instructions unresolved\n"); + if (uninstall_steps_succeeded(openclaw_errors_before)) { + printf(" %s workspace instructions + compaction augmentation\n", + uninstall_verb(dry_run)); + } + } else if (uninstall_steps_succeeded(openclaw_errors_before)) { + printf(" %s compaction augmentation; workspace instructions unresolved\n", + uninstall_verb(dry_run)); } } } @@ -11741,7 +11817,7 @@ static void uninstall_additional_agents(const cbm_detected_agents_t *agents, con } else if (hook_result != CBM_YAML_IDENTITY_EDIT_OK) { record_agent_config_error(true, "Hermes", "pre_llm_hook_uninstall", cp); } else { - printf(" hook: removed canonical pre_llm_call entry\n"); + printf(" hook: %s canonical pre_llm_call entry\n", uninstall_verb(dry_run)); } uninstall_agent_mcp_instr((mcp_uninstall_args_t){"Hermes", cp, NULL}, dry_run, cbm_remove_hermes_mcp_owned); @@ -11754,7 +11830,8 @@ static void uninstall_additional_agents(const cbm_detected_agents_t *agents, con snprintf(skills_dir, sizeof(skills_dir), "%s/.agents/skills", home); uninstall_agent_mcp_instr((mcp_uninstall_args_t){"OpenHands", cp, NULL}, dry_run, cbm_remove_editor_mcp_owned); - printf(" removed %d skill(s)\n", cbm_remove_skills(skills_dir, dry_run)); + printf(" %d skill(s) %s\n", cbm_remove_skills(skills_dir, dry_run), + dry_run ? "would be removed" : "removed"); } if (agents->augment) { char cp[CLI_BUF_1K]; @@ -11776,6 +11853,7 @@ static void uninstall_additional_agents(const cbm_detected_agents_t *agents, con cbm_agent_installed_binary_path(home, binary_path, sizeof(binary_path)); uninstall_agent_mcp_instr((mcp_uninstall_args_t){"Augment/Auggie", cp, ip}, dry_run, cbm_remove_editor_mcp_owned); + int augment_errors_before = g_agent_uninstall_errors; uninstall_tiered_agent_profiles( (cbm_tiered_profile_set_t){ .label = "Augment/Auggie", @@ -11817,7 +11895,10 @@ static void uninstall_additional_agents(const cbm_detected_agents_t *agents, con } } } - printf(" removed SessionStart + PostToolUse hooks + dedicated subagent\n"); + if (uninstall_steps_succeeded(augment_errors_before)) { + printf(" %s SessionStart + PostToolUse hooks + dedicated subagent\n", + uninstall_verb(dry_run)); + } } if (agents->cline) { char cline_root[CLI_BUF_1K]; @@ -11903,6 +11984,7 @@ static void uninstall_additional_agents(const cbm_detected_agents_t *agents, con snprintf(skills_dir, sizeof(skills_dir), "%s/.factory/skills", home); uninstall_agent_mcp_instr((mcp_uninstall_args_t){"Factory Droid", cp, ip}, dry_run, cbm_remove_factory_mcp_owned); + int factory_errors_before = g_agent_uninstall_errors; if (!dry_run && cbm_remove_factory_hooks(hp, installed_binary) != CLI_OK) { record_agent_config_error(true, "Factory Droid", "context_hook_uninstall", hp); } @@ -11915,7 +11997,9 @@ static void uninstall_additional_agents(const cbm_detected_agents_t *agents, con .dialect = CBM_GRAPH_DIALECT_FACTORY, }, dry_run); - printf(" removed SessionStart + PostToolUse hooks\n"); + if (uninstall_steps_succeeded(factory_errors_before)) { + printf(" %s SessionStart + PostToolUse hooks\n", uninstall_verb(dry_run)); + } } if (agents->crush) { char cp[CLI_BUF_1K]; @@ -12073,13 +12157,18 @@ static int cli_uninstall_activate(void *opaque) { uninstall_additional_agents(&activation->agents, activation->home, activation->dry_run); uninstall_agent_client_registry(activation->home, activation->dry_run); - if (g_agent_uninstall_errors != 0) { - cli_activation_transaction_abort_or_fail_stop(&activation->binary_transaction, - "uninstall_transaction_config_cleanup_abort"); - (void)fprintf(stderr, "error: one or more agent cleanup operations failed; executable " - "and index removal were not started\n"); - return CLI_ACTIVATION_PARTIAL; - } + /* An agent configuration that could not be cleaned up used to stop the run + * right here, before the executable and the indexes were touched. One + * unwritable file — a Cursor config symlinked into a dotfiles repository, + * or an agent detected by its binary on PATH whose config this HOME never + * had — then left a 282 MB executable and a 104 MB index cache behind after + * a run that printed "Uninstall complete" (#1954). + * + * Cleaning up an agent's configuration and removing this tool's own files + * are separate jobs. A failure in the first no longer cancels the second. + * Every failure is still named on stderr as it happens, counted, summarised + * at the end of the run, and still makes the exit code non-zero, so nothing + * about the failure becomes quieter — only the blast radius shrinks. */ if (activation->delete_indexes && !activation->dry_run) { int expected = count_db_indexes(activation->home); @@ -12252,8 +12341,21 @@ int cbm_cmd_uninstall(int argc, char **argv) { return CLI_TRUE; } - printf("\nUninstall complete. Please restart your coding-agent sessions " - "to properly take this into account.\n"); + if (g_agent_uninstall_errors == 0) { + printf("\nUninstall complete. Please restart your coding-agent sessions " + "to properly take this into account.\n"); + } else { + /* Say the count out loud. The per-failure lines are on stderr, which a + * reader who only watches stdout never sees, and the run is no longer + * stopped by them — so this is the one place that tells the reader some + * configuration files still hold entries of ours. */ + printf("\nUninstall finished, and %d agent configuration cleanup step(s) failed.\n" + "Look for the \"error: agent_config\" lines above: those files still need an\n" + "edit by hand. Removal of the executable and the indexes went ahead anyway.\n" + "Please restart your coding-agent sessions to properly take this into " + "account.\n", + g_agent_uninstall_errors); + } if (dry_run) { printf("(dry-run — no files were modified)\n"); } diff --git a/tests/test_cli.c b/tests/test_cli.c index 976280868..fb331d90a 100644 --- a/tests/test_cli.c +++ b/tests/test_cli.c @@ -2277,6 +2277,198 @@ TEST(cli_uninstall_preserves_binary_and_index_when_cohort_does_not_drain) { PASS(); } +/* #1954: one agent config that cannot be cleaned up must not keep the + * executable and the indexes on disk. + * + * The fixture is the reporter's own setup: ~/.cursor/mcp.json is a symlink into + * a dotfiles directory, which the JSON editors refuse to write through. Before + * the fix that single refusal returned early, so a run that printed + * "Uninstall complete" left a 282 MB executable and the whole index cache + * behind. PATH moves with HOME so a real agent binary on the developer's + * machine cannot add a second, unrelated failure. */ +#ifndef _WIN32 +TEST(cli_uninstall_removes_binary_and_index_despite_agent_config_failure) { + char tmpdir[256]; + snprintf(tmpdir, sizeof(tmpdir), "/tmp/cli-uninstall-linked-XXXXXX"); + if (!cbm_mkdtemp(tmpdir)) { + FAIL("cbm_mkdtemp failed"); + } + char *old_home = NULL; + char *old_cache = NULL; + cli_activation_save_env(&old_home, &old_cache); + cbm_setenv("HOME", tmpdir, 1); + char *old_path = save_test_env("PATH"); + cbm_setenv("PATH", tmpdir, 1); + + char dotfiles_dir[512]; + char cursor_dir[512]; + char link_target[640]; + char cursor_config[640]; + snprintf(dotfiles_dir, sizeof(dotfiles_dir), "%s/dotfiles", tmpdir); + snprintf(cursor_dir, sizeof(cursor_dir), "%s/.cursor", tmpdir); + test_mkdirp(dotfiles_dir); + test_mkdirp(cursor_dir); + snprintf(link_target, sizeof(link_target), "%s/mcp.json", dotfiles_dir); + snprintf(cursor_config, sizeof(cursor_config), "%s/mcp.json", cursor_dir); + write_test_file(link_target, "{\"mcpServers\":{}}\n"); + if (symlink(link_target, cursor_config) != 0) { + cli_activation_restore_env(old_home, old_cache); + restore_test_env("PATH", old_path); + test_rmdir_r(tmpdir); + FAIL("failed to create the linked Cursor config fixture"); + } + + char cache_dir[512]; + char index_path[640]; + snprintf(cache_dir, sizeof(cache_dir), "%s/cache", tmpdir); + cbm_setenv("CBM_CACHE_DIR", cache_dir, 1); + test_mkdirp(cache_dir); + snprintf(index_path, sizeof(index_path), "%s/project.db", cache_dir); + write_test_file(index_path, "index must not survive a linked agent config"); + + char bin_dir[512]; + char bin_target[640]; + snprintf(bin_dir, sizeof(bin_dir), "%s/.local/bin", tmpdir); + test_mkdirp(bin_dir); + snprintf(bin_target, sizeof(bin_target), "%s/codebase-memory-mcp", bin_dir); + write_test_file(bin_target, "binary must not survive a linked agent config"); + + cli_activation_fake_t fake = { + .participants_active = true, + .mutation_reserve_result = 1, + }; + cbm_cli_activation_ops_t ops = cli_activation_fake_ops(&fake); + cbm_cli_set_activation_ops_for_test(&ops); + + FILE *capture = tmpfile(); + int saved_stdout = capture ? dup(STDOUT_FILENO) : -1; + int saved_stderr = capture ? dup(STDERR_FILENO) : -1; + int rc = -1; + if (capture && saved_stdout >= 0 && saved_stderr >= 0) { + fflush(NULL); + if (dup2(fileno(capture), STDOUT_FILENO) >= 0 && + dup2(fileno(capture), STDERR_FILENO) >= 0) { + char *argv[] = {"--yes"}; + rc = cli_test_cmd_uninstall(1, argv); + } + fflush(NULL); + (void)dup2(saved_stdout, STDOUT_FILENO); + (void)dup2(saved_stderr, STDERR_FILENO); + } + if (saved_stdout >= 0) { + close(saved_stdout); + } + if (saved_stderr >= 0) { + close(saved_stderr); + } + char output[8192] = {0}; + if (capture) { + rewind(capture); + size_t count = fread(output, 1, sizeof(output) - 1U, capture); + output[count] = '\0'; + fclose(capture); + } + cbm_cli_set_activation_ops_for_test(NULL); + cbm_set_auto_answer_for_test(0); + + bool index_left = read_test_file(index_path) != NULL; + bool binary_left = read_test_file(bin_target) != NULL; + bool link_intact = read_test_file(link_target) != NULL; + bool cleanup_failure_reported = + strstr(output, "agent configuration cleanup step(s) failed") != NULL; + bool old_abort_message = + strstr(output, "executable and index removal were not started") != NULL; + cli_activation_restore_env(old_home, old_cache); + restore_test_env("PATH", old_path); + test_rmdir_r(tmpdir); + + /* The failed config cleanup still fails the run. */ + ASSERT_EQ(rc, 1); + ASSERT_FALSE(index_left); + ASSERT_FALSE(binary_left); + /* The link's target was never written through. */ + ASSERT_TRUE(link_intact); + ASSERT_TRUE(cleanup_failure_reported); + ASSERT_FALSE(old_abort_message); + PASS(); +} +#endif + +/* #1954, second half: a --dry-run must not describe removals it did not make. + * Every per-client line used to be written in the past tense, so a dry run read + * as a report of a dozen rewritten configuration files. */ +TEST(cli_uninstall_dry_run_report_uses_the_future_tense) { + char tmpdir[256]; + snprintf(tmpdir, sizeof(tmpdir), "/tmp/cli-uninstall-tense-XXXXXX"); + if (!cbm_mkdtemp(tmpdir)) { + FAIL("cbm_mkdtemp failed"); + } + char *old_home = NULL; + char *old_cache = NULL; + cli_activation_save_env(&old_home, &old_cache); + cbm_setenv("HOME", tmpdir, 1); + char *old_path = save_test_env("PATH"); + cbm_setenv("PATH", tmpdir, 1); + + char cursor_dir[512]; + char cursor_config[640]; + snprintf(cursor_dir, sizeof(cursor_dir), "%s/.cursor", tmpdir); + test_mkdirp(cursor_dir); + snprintf(cursor_config, sizeof(cursor_config), "%s/mcp.json", cursor_dir); + const char *before = "{\"mcpServers\":{}}\n"; + write_test_file(cursor_config, before); + + char cache_dir[512]; + snprintf(cache_dir, sizeof(cache_dir), "%s/cache", tmpdir); + cbm_setenv("CBM_CACHE_DIR", cache_dir, 1); + test_mkdirp(cache_dir); + + FILE *capture = tmpfile(); + int saved_stdout = capture ? dup(STDOUT_FILENO) : -1; + int saved_stderr = capture ? dup(STDERR_FILENO) : -1; + int rc = -1; + if (capture && saved_stdout >= 0 && saved_stderr >= 0) { + fflush(NULL); + if (dup2(fileno(capture), STDOUT_FILENO) >= 0 && + dup2(fileno(capture), STDERR_FILENO) >= 0) { + char *argv[] = {"--dry-run", "--yes"}; + rc = cli_test_cmd_uninstall(2, argv); + } + fflush(NULL); + (void)dup2(saved_stdout, STDOUT_FILENO); + (void)dup2(saved_stderr, STDERR_FILENO); + } + if (saved_stdout >= 0) { + close(saved_stdout); + } + if (saved_stderr >= 0) { + close(saved_stderr); + } + char output[8192] = {0}; + if (capture) { + rewind(capture); + size_t count = fread(output, 1, sizeof(output) - 1U, capture); + output[count] = '\0'; + fclose(capture); + } + cbm_set_auto_answer_for_test(0); + + char *after = read_test_file_alloc(cursor_config); + bool config_untouched = after && strcmp(after, before) == 0; + free(after); + bool says_would_remove = strstr(output, "Cursor: would remove MCP config entry") != NULL; + bool claims_a_past_removal = strstr(output, "Cursor: removed MCP config entry") != NULL; + cli_activation_restore_env(old_home, old_cache); + restore_test_env("PATH", old_path); + test_rmdir_r(tmpdir); + + ASSERT_EQ(rc, 0); + ASSERT_TRUE(config_untouched); + ASSERT_TRUE(says_would_remove); + ASSERT_FALSE(claims_a_past_removal); + PASS(); +} + TEST(cli_activation_guard_is_bypassed_for_dry_run_and_plan) { char tmpdir[256]; snprintf(tmpdir, sizeof(tmpdir), "/tmp/cli-daemon-stateless-XXXXXX"); @@ -4613,6 +4805,11 @@ TEST(cli_agent_install_reports_safe_editor_refusal) { PASS(); } +/* A safe-editor refusal still fails the run, and it no longer keeps the + * executable. Removing an agent's configuration and removing this tool's own + * files are separate jobs, so one malformed OpenClaw config does not cancel the + * other (#1954). The two claims that mattered before are unchanged: the exit + * code is non-zero, and the malformed file is never written through. */ TEST(cli_agent_uninstall_reports_safe_editor_refusal) { char tmpdir[256]; snprintf(tmpdir, sizeof(tmpdir), "/tmp/cli-uninstall-refusal-XXXXXX"); @@ -4635,7 +4832,7 @@ TEST(cli_agent_uninstall_reports_safe_editor_refusal) { #else snprintf(bin_path, sizeof(bin_path), "%s/codebase-memory-mcp", bin_dir); #endif - write_test_file(bin_path, "installed binary must remain live\n"); + write_test_file(bin_path, "installed binary must not outlive the uninstall\n"); char *saved_home = save_test_env("HOME"); char *saved_path = save_test_env("PATH"); @@ -4659,9 +4856,10 @@ TEST(cli_agent_uninstall_reports_safe_editor_refusal) { restore_test_env("HOME", saved_home); restore_test_env("PATH", saved_path); test_rmdir_r(tmpdir); - if (rc == 0 || !preserved || !binary_preserved || fake.mutation_reserve_count != 1 || - fake.mutation_lease_release_count != 1 || !strstr(fake.diagnostic, "executable was kept")) - FAIL("agent uninstall refusal must fail before removing the live binary"); + if (rc == 0 || !preserved || binary_preserved || fake.mutation_reserve_count != 1 || + fake.mutation_lease_release_count != 1 || fake.diagnostic[0] != '\0') + FAIL("agent uninstall refusal must fail the run, keep the malformed config untouched, " + "and still remove the executable"); PASS(); } @@ -10618,7 +10816,8 @@ TEST(cli_codex_migrates_to_single_hook_representation) { #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"); + /* #1954: a failed agent-config cleanup no longer keeps the executable. */ + write_test_file(binary_path, "installed binary must not survive a failed cleanup\n"); char *saved_home = save_test_env("HOME"); char *saved_path = save_test_env("PATH"); @@ -10673,7 +10872,7 @@ TEST(cli_codex_migrates_to_single_hook_representation) { struct stat state; hooks = read_test_file_alloc(hooks_path); char *agents_after_uninstall = read_test_file_alloc(agents_path); - bool independent_cleanup = uninstall_rc != 0 && stat(binary_path, &state) == 0 && + 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") && agents_after_uninstall && agents_after_uninstall[0] == '\0'; @@ -14793,6 +14992,10 @@ SUITE(cli) { RUN_TEST(cli_update_agent_configs_finish_before_guard_release); RUN_TEST(cli_uninstall_quiesces_active_cohort_before_removing_binary_and_index); RUN_TEST(cli_uninstall_preserves_binary_and_index_when_cohort_does_not_drain); +#ifndef _WIN32 + RUN_TEST(cli_uninstall_removes_binary_and_index_despite_agent_config_failure); +#endif + RUN_TEST(cli_uninstall_dry_run_report_uses_the_future_tense); RUN_TEST(cli_activation_guard_is_bypassed_for_dry_run_and_plan); #ifdef _WIN32 RUN_TEST(cli_windows_update_hands_off_to_install_script);