diff --git a/src/cli/cli.c b/src/cli/cli.c index c58738a42..37cb397d8 100644 --- a/src/cli/cli.c +++ b/src/cli/cli.c @@ -5294,6 +5294,53 @@ static bool cbm_install_subagent_reminder_script(const char *home, const char *b return cbm_write_owned_hook_script_with_legacy(script_path, script, legacy, 1U); } +/* #1387 dry-run predicate: would writing this hook script succeed, or would + * the owned-document migration refuse it because the file on disk is not ours? + * Read-only — it must never touch the filesystem, since a dry run promises + * exactly that. An unreadable/unsafe state counts as "would not succeed": the + * preview should warn rather than promise. */ +static bool cbm_hook_script_write_would_succeed(const char *home, const char *binary_path, + const char *script_name) { + if (!home || !binary_path || !script_name) { + return false; + } + char config_dir[CLI_BUF_1K]; + cbm_claude_config_dir(home, config_dir, sizeof(config_dir)); + if (!config_dir[0]) { + return false; + } + char script_path[CLI_BUF_1K]; + int written = + snprintf(script_path, sizeof(script_path), "%s/hooks/%s", config_dir, script_name); + if (written <= 0 || (size_t)written >= sizeof(script_path)) { + return false; + } + const char *prefix = cmm_gate_script_prefix; + if (strcmp(script_name, CMM_SESSION_REMINDER_SCRIPT) == 0) { + prefix = cmm_session_script_prefix; + } else if (strcmp(script_name, CMM_SUBAGENT_REMINDER_SCRIPT) == 0) { + prefix = cmm_subagent_script_prefix; + } + char script[CLI_BUF_8K]; + if (cbm_build_current_hook_script(prefix, binary_path, script, sizeof(script)) != CLI_OK) { + return false; + } + /* Released shapes are accepted by the real write, so they must be accepted + * here too or the preview would warn about a script that upgrades fine. */ + char released[CLI_BUF_8K]; + const char *candidates[2]; + size_t candidate_count = 0U; + if (strcmp(script_name, CMM_HOOK_GATE_SCRIPT) == 0 && + cbm_build_released_gate_script(binary_path, released, sizeof(released)) == CLI_OK) { + candidates[candidate_count++] = released; + } else if (strcmp(script_name, CMM_SESSION_REMINDER_SCRIPT) == 0) { + candidates[candidate_count++] = cmm_released_session_script; + } else if (strcmp(script_name, CMM_SUBAGENT_REMINDER_SCRIPT) == 0) { + candidates[candidate_count++] = cmm_released_subagent_script; + } + return cbm_text_owned_document_status(script_path, script, candidates, candidate_count) == 0; +} + int cbm_upsert_claude_subagent_hooks(const char *settings_path) { char command[CLI_BUF_8K]; char previous_command[CLI_BUF_8K]; @@ -7191,6 +7238,18 @@ static void install_claude_code_config(const char *home, const char *binary_path bool gate_ok = dry_run; bool session_ok = dry_run; bool subagent_ok = dry_run; + if (dry_run) { + /* #1387 (second half): the dry run claimed EVERY hook group as + * installable because these flags were simply `true`. When the on-disk + * script is not ours the real install refuses to rewrite it, so the + * preview promised what the run could not deliver and the reporter had + * no way to see the loss coming. Predict each refusal read-only. */ + gate_ok = cbm_hook_script_write_would_succeed(home, binary_path, CMM_HOOK_GATE_SCRIPT); + session_ok = + cbm_hook_script_write_would_succeed(home, binary_path, CMM_SESSION_REMINDER_SCRIPT); + subagent_ok = + cbm_hook_script_write_would_succeed(home, binary_path, CMM_SUBAGENT_REMINDER_SCRIPT); + } if (!dry_run) { char hook_path[CLI_BUF_1K]; gate_ok = cbm_install_hook_gate_script(home, binary_path); @@ -7237,6 +7296,22 @@ static void install_claude_code_config(const char *home, const char *binary_path if (subagent_ok) { printf(" hooks: SubagentStart (MCP usage reminder for subagents)\n"); } + if (dry_run) { + /* Name every script the real run would refuse. Silence is what made + * #1387 invisible in advance: the group simply went unmentioned, which + * reads as "nothing to do" rather than "this will be skipped". */ + static const char *const preview_scripts[] = { + CMM_HOOK_GATE_SCRIPT, CMM_SESSION_REMINDER_SCRIPT, CMM_SUBAGENT_REMINDER_SCRIPT}; + const bool preview_ok[] = {gate_ok, session_ok, subagent_ok}; + for (size_t i = 0U; i < sizeof(preview_scripts) / sizeof(preview_scripts[0]); i++) { + if (!preview_ok[i]) { + printf(" hooks: %s/hooks/%s would be skipped — the file there is not ours " + "(modified, or written by another install), so the rewrite is refused " + "and existing hook entries are left untouched\n", + config_dir, preview_scripts[i]); + } + } + } /* Migration nudge: when CLAUDE_CONFIG_DIR is set and a legacy ~/.claude tree * still exists, mention it so users can clean up stale artifacts. */ diff --git a/src/cli/config_text_edit.c b/src/cli/config_text_edit.c index 27fa3fd1c..cee66bdfe 100644 --- a/src/cli/config_text_edit.c +++ b/src/cli/config_text_edit.c @@ -1222,6 +1222,58 @@ static int text_matches_candidate(const char *data, size_t data_len, const char return TEXT_OK; } +/* Read-only classification: WOULD a migrate of this document succeed? + * Returns TEXT_OK when the path is absent or already byte-identical to the + * current or an exact released document (a write would proceed), TEXT_UNOWNED + * when it holds other bytes (a write would be refused), TEXT_ERROR on unsafe + * state. Mirrors text_migrate_owned_document's decision WITHOUT writing, so + * `install --dry-run` can predict a refusal instead of promising an install it + * cannot deliver (#1387). */ +int cbm_text_owned_document_status(const char *file_path, const char *current_content, + const char *const *released_contents, size_t released_count) { + size_t current_len = 0U; + if (!text_valid_path(file_path) || + text_bounded_strlen(current_content, TEXT_MAX_BYTES, ¤t_len) != TEXT_OK || + text_validate_bytes(current_content, current_len, 1) != TEXT_OK || + (released_count > 0U && !released_contents)) { + return TEXT_ERROR; + } + + char *old_data = NULL; + size_t old_len = 0U; + text_file_snapshot_t snapshot; + if (text_read_file(file_path, &old_data, &old_len, &snapshot) != TEXT_OK || + text_validate_bytes(old_data, old_len, 1) != TEXT_OK) { + free(old_data); + return TEXT_ERROR; + } + if (!snapshot.exists) { + free(old_data); + return TEXT_OK; /* absent -> the write creates it */ + } + bool matches = false; + if (text_matches_candidate(old_data, old_len, current_content, &matches) != TEXT_OK) { + free(old_data); + return TEXT_ERROR; + } + if (matches) { + free(old_data); + return TEXT_OK; + } + for (size_t i = 0U; i < released_count; i++) { + if (text_matches_candidate(old_data, old_len, released_contents[i], &matches) != TEXT_OK) { + free(old_data); + return TEXT_ERROR; + } + if (matches) { + free(old_data); + return TEXT_OK; /* an exact released doc upgrades */ + } + } + free(old_data); + return TEXT_UNOWNED; +} + static int text_migrate_owned_document(const char *file_path, const char *current_content, const char *const *released_contents, size_t released_count, int override_mode, unsigned int requested_mode) { diff --git a/src/cli/config_text_edit.h b/src/cli/config_text_edit.h index d075a331b..3f6e7c425 100644 --- a/src/cli/config_text_edit.h +++ b/src/cli/config_text_edit.h @@ -29,6 +29,11 @@ int cbm_text_ensure_owned_document(const char *file_path, const char *owned_cont /* Create current content, preserve it when already current, or atomically * upgrade only an exact byte-for-byte previously released document. Returns * 1 for user-modified/unowned content. */ +/* Read-only: would a migrate succeed? 0 = yes (absent/current/exact released), + * 1 = refused (other bytes present), -1 = unsafe. Lets `install --dry-run` + * predict a refusal instead of promising an install it cannot deliver. */ +int cbm_text_owned_document_status(const char *file_path, const char *current_content, + const char *const *released_contents, size_t released_count); int cbm_text_migrate_owned_document(const char *file_path, const char *current_content, const char *const *released_contents, size_t released_count); /* As above, but publish the owned document with the requested low permission diff --git a/tests/test_cli.c b/tests/test_cli.c index 625ed83a3..f35dd9d4c 100644 --- a/tests/test_cli.c +++ b/tests/test_cli.c @@ -5955,6 +5955,80 @@ TEST(cli_detected_agent_summary_includes_registry_clients) { } #endif +#ifndef _WIN32 +/* Regression for #1387 (second half): `install --dry-run` printed all three + * hook groups as if they would install, even when the on-disk hook script is + * NOT ours and the real run would refuse to rewrite it. The dry run is exactly + * where that has to be visible - the reporter could not see the loss coming. */ +TEST(cli_dry_run_predicts_refused_hook_script_issue1387) { + char tmpdir[256]; + snprintf(tmpdir, sizeof(tmpdir), "/tmp/cli-dryrun-refusal-XXXXXX"); + if (!cbm_mkdtemp(tmpdir)) + FAIL("cbm_mkdtemp failed"); + + char hooks_dir[512]; + snprintf(hooks_dir, sizeof(hooks_dir), "%s/.claude/hooks", tmpdir); + if (!cbm_mkdir_p(hooks_dir, 0755)) + FAIL("mkdir hooks_dir failed"); + /* A gate script that is NOT ours: a manual install pointing at another + * binary. The real install refuses to rewrite it (TEXT_UNOWNED). */ + char gate_path[768]; + snprintf(gate_path, sizeof(gate_path), "%s/cbm-code-discovery-gate", hooks_dir); + write_test_file(gate_path, "#!/usr/bin/env bash\n" + "# codebase-memory-mcp search augmenter (Claude Code PreToolUse).\n" + "BIN=\"/opt/tools/cbm/codebase-memory-mcp\"\n" + "exec 0\n"); + + const char *const env_names[] = {"HOME", "PATH", "CLAUDE_CONFIG_DIR"}; + char *saved[sizeof(env_names) / sizeof(env_names[0])]; + for (size_t i = 0U; i < sizeof(env_names) / sizeof(env_names[0]); i++) { + saved[i] = save_test_env(env_names[i]); + cbm_unsetenv(env_names[i]); + } + cbm_setenv("HOME", tmpdir, 1); + cbm_setenv("PATH", tmpdir, 1); + + FILE *capture = tmpfile(); + int saved_stdout = capture ? dup(STDOUT_FILENO) : -1; + bool redirected = false; + if (capture && saved_stdout >= 0) { + fflush(stdout); + redirected = dup2(fileno(capture), STDOUT_FILENO) >= 0; + } + if (redirected) { + (void)cbm_install_agent_configs(tmpdir, "/opt/other/codebase-memory-mcp", false, true); + fflush(stdout); + (void)dup2(saved_stdout, STDOUT_FILENO); + } + if (saved_stdout >= 0) { + close(saved_stdout); + } + char output[16384] = {0}; + if (capture) { + rewind(capture); + size_t count = fread(output, 1, sizeof(output) - 1U, capture); + output[count] = '\0'; + fclose(capture); + } + + /* The dry run must WARN about the script it cannot rewrite, and must not + * claim the search-augmentation hook group as installable. */ + bool warned = strstr(output, "cbm-code-discovery-gate") != NULL && + (strstr(output, "not ours") != NULL || + strstr(output, "would be skipped") != NULL || strstr(output, "refuse") != NULL); + + for (size_t i = 0U; i < sizeof(env_names) / sizeof(env_names[0]); i++) { + restore_test_env(env_names[i], saved[i]); + } + test_rmdir_r(tmpdir); + if (!redirected) + FAIL("stdout capture failed"); + if (!warned) + FAIL("dry-run must predict a refused hook-script rewrite (#1387)"); + PASS(); +} +#endif + TEST(cli_agent_client_registry_routes_plan_install_and_uninstall) { char tmpdir[256]; snprintf(tmpdir, sizeof(tmpdir), "/tmp/cli-agent-registry-XXXXXX"); @@ -12117,6 +12191,9 @@ SUITE(cli) { RUN_TEST(cli_hermes_stable_shell_context_contract); #ifndef _WIN32 RUN_TEST(cli_detected_agent_summary_includes_registry_clients); +#endif +#ifndef _WIN32 + RUN_TEST(cli_dry_run_predicts_refused_hook_script_issue1387); #endif RUN_TEST(cli_agent_client_registry_routes_plan_install_and_uninstall); RUN_TEST(cli_registry_installs_kimi_rovo_amp_durable_context);