diff --git a/src/cli/cli.c b/src/cli/cli.c index 4228dbaa..feb4f5b2 100644 --- a/src/cli/cli.c +++ b/src/cli/cli.c @@ -349,6 +349,48 @@ int cbm_copy_file(const char *src, const char *dst) { return rc == 0 ? 0 : CLI_ERR; } +/* Return true if two paths refer to the same on-disk file. Used to avoid + * copying the running binary onto itself during install (cbm_copy_file would + * truncate it, since it opens the destination "wb" before reading the source). */ +static bool cbm_same_file(const char *a, const char *b) { + struct stat sa; + struct stat sb; + if (stat(a, &sa) != 0 || stat(b, &sb) != 0) { + return false; + } +#ifdef _WIN32 + /* st_ino is unreliable on Windows; compare normalized path strings. */ + char na[CLI_BUF_1K]; + char nb[CLI_BUF_1K]; + snprintf(na, sizeof(na), "%s", a); + snprintf(nb, sizeof(nb), "%s", b); + cbm_normalize_path_sep(na); + cbm_normalize_path_sep(nb); + return strcmp(na, nb) == 0; +#else + return sa.st_dev == sb.st_dev && sa.st_ino == sb.st_ino; +#endif +} + +/* Copy the running binary into the canonical install target, preserving the + * executable bit. When src and dst are the same on-disk file the copy is + * skipped: cbm_copy_file opens dst "wb" before reading src, so copying a file + * onto itself would truncate it to zero. Returns 0 on success or skip, + * CLI_ERR on failure. Exposed (non-static) as the regression surface for the + * `install --force` binary-swap bug (#472). */ +int cbm_copy_binary_to_target(const char *src, const char *dst) { + if (cbm_same_file(src, dst)) { + return 0; /* already in place — nothing to copy */ + } + if (cbm_copy_file(src, dst) != 0) { + return CLI_ERR; + } +#ifndef _WIN32 + (void)chmod(dst, CLI_OCTAL_PERM); +#endif + return 0; +} + /* Replace a binary file. Unlinks the old file first (handles read-only and * running binaries on Unix where unlink succeeds on open files). On all * platforms, the caller should tell the user to restart after update. */ @@ -3412,24 +3454,68 @@ int cbm_cmd_install(int argc, char **argv) { } } - /* Step 1c: macOS ad-hoc signing (in case binary was placed without signing) */ + /* Step 1c: Place the running binary at the canonical install target. + * Previously install only re-signed whatever was already at the target, so + * `install --force` from a freshly built binary silently kept the OLD file + * — operators ran stale code believing they had upgraded (#472). Copy the + * running binary to ~/.local/bin (unless we ARE that file), then sign it. */ + char self_path[CLI_BUF_1K] = {0}; + cbm_detect_self_path(self_path, sizeof(self_path), home); + + char bin_target[CLI_BUF_1K]; +#ifdef _WIN32 + snprintf(bin_target, sizeof(bin_target), "%s/.local/bin/codebase-memory-mcp.exe", home); +#else + snprintf(bin_target, sizeof(bin_target), "%s/.local/bin/codebase-memory-mcp", home); +#endif + + if (!cbm_same_file(self_path, bin_target)) { + struct stat tgt_st; + bool target_exists = (stat(bin_target, &tgt_st) == 0); + bool do_copy = !target_exists || force; + if (target_exists && !force) { + printf("A different binary already exists at:\n %s\n", bin_target); + if (prompt_yn("Replace it with the binary you ran install from?")) { + do_copy = true; + force = true; /* user approved replacement for this run */ + } else { + printf("Keeping existing binary; configs will point at it.\n\n"); + } + } + if (do_copy) { + char bin_dir[CLI_BUF_1K]; + snprintf(bin_dir, sizeof(bin_dir), "%s/.local/bin", home); + if (dry_run) { + printf("Would install binary -> %s\n\n", bin_target); + } else { + cbm_mkdir_p(bin_dir, CLI_OCTAL_PERM); + if (cbm_copy_binary_to_target(self_path, bin_target) != 0) { + (void)fprintf(stderr, "error: failed to copy binary to %s\n", bin_target); + return CLI_TRUE; + } + printf("Installed binary -> %s\n\n", bin_target); + } + } + } + + /* Step 1d: macOS ad-hoc signing of the installed binary. A freshly + * clang-built arm64 binary is linker-signed (flags=0x20002) and gets + * Killed:9 when spawned by an MCP host; re-signing ad-hoc (flags=0x2) + * makes it launchable. Sign the target, not whatever the operator ran. */ #ifdef __APPLE__ - { - char sign_path[CLI_BUF_1K]; - snprintf(sign_path, sizeof(sign_path), "%s/.local/bin/codebase-memory-mcp", home); + if (!dry_run) { struct stat sign_st; - if (stat(sign_path, &sign_st) == 0) { - (void)cbm_macos_adhoc_sign(sign_path); + if (stat(bin_target, &sign_st) == 0) { + if (cbm_macos_adhoc_sign(bin_target) != 0) { + (void)fprintf( + stderr, "warning: ad-hoc signing failed — binary may not run on macOS arm64\n"); + } } } #endif - /* Step 2: Binary path — detect actual location at runtime. */ - char self_path[CLI_BUF_1K] = {0}; - cbm_detect_self_path(self_path, sizeof(self_path), home); - - /* Step 3: Install/refresh all agent configs */ - cbm_install_agent_configs(home, self_path, force, dry_run); + /* Step 3: Install/refresh all agent configs, pointing at the install target. */ + cbm_install_agent_configs(home, bin_target, force, dry_run); /* Step 4: Ensure PATH */ char bin_dir[CLI_BUF_1K]; diff --git a/src/cli/cli.h b/src/cli/cli.h index 384af45a..9efe6789 100644 --- a/src/cli/cli.h +++ b/src/cli/cli.h @@ -45,6 +45,12 @@ const char *cbm_find_cli(const char *name, const char *home_dir); /* Copy a file from src to dst. Returns 0 on success, -1 on error. */ int cbm_copy_file(const char *src, const char *dst); +/* Copy the running binary into the canonical install target, preserving the + * executable bit, skipping the copy when src and dst are the same file (which + * would otherwise truncate the running binary). Returns 0 on success or skip, + * -1 on error. Regression surface for the install --force binary-swap bug. */ +int cbm_copy_binary_to_target(const char *src, const char *dst); + /* Replace a binary file: unlinks the existing file first (handles read-only), * then creates a new file with the given data and permissions. * Returns 0 on success, -1 on error. */ diff --git a/tests/test_cli.c b/tests/test_cli.c index add43138..0b78537c 100644 --- a/tests/test_cli.c +++ b/tests/test_cli.c @@ -1026,6 +1026,82 @@ TEST(cli_copy_file_source_not_found) { PASS(); } +/* #472: install --force must copy the freshly-built binary to the target and + * make it executable — previously it re-signed whatever was already there. */ +TEST(cli_install_copies_binary_to_target_issue472) { + char tmpdir[256]; + snprintf(tmpdir, sizeof(tmpdir), "/tmp/cli-binswap-XXXXXX"); + if (!cbm_mkdtemp(tmpdir)) + FAIL("cbm_mkdtemp failed"); + + char src[512], dst[512]; + snprintf(src, sizeof(src), "%s/new-build", tmpdir); + snprintf(dst, sizeof(dst), "%s/installed", tmpdir); + + write_test_file(src, "fresh build bytes"); + + /* Target does not exist yet → must be created with the source content. */ + int rc = cbm_copy_binary_to_target(src, dst); + ASSERT_EQ(rc, 0); + + const char *data = read_test_file(dst); + ASSERT_STR_EQ(data, "fresh build bytes"); + +#ifndef _WIN32 + /* The exec bit is set via chmod, which is POSIX-only; on Windows it is not + * meaningful and MinGW stat() derives it from the file extension. */ + struct stat st; + ASSERT_EQ(stat(dst, &st), 0); + ASSERT((st.st_mode & S_IXUSR) != 0); /* executable bit set */ +#endif + + /* Overwrite an existing (stale) target with new content. */ + write_test_file(dst, "STALE"); + write_test_file(src, "upgraded build bytes"); + rc = cbm_copy_binary_to_target(src, dst); + ASSERT_EQ(rc, 0); + data = read_test_file(dst); + ASSERT_STR_EQ(data, "upgraded build bytes"); + + test_rmdir_r(tmpdir); + PASS(); +} + +/* #472: copying the running binary onto itself must NOT truncate it. */ +TEST(cli_install_same_file_guard_issue472) { + char tmpdir[256]; + snprintf(tmpdir, sizeof(tmpdir), "/tmp/cli-samefile-XXXXXX"); + if (!cbm_mkdtemp(tmpdir)) + FAIL("cbm_mkdtemp failed"); + + char path[512]; + snprintf(path, sizeof(path), "%s/self", tmpdir); + write_test_file(path, "must survive self-copy"); + + int rc = cbm_copy_binary_to_target(path, path); + ASSERT_EQ(rc, 0); /* skipped, not failed */ + + const char *data = read_test_file(path); + ASSERT_STR_EQ(data, "must survive self-copy"); /* intact, not zeroed */ + +#ifndef _WIN32 + /* Distinct path strings resolving to the same inode (a symlink — exactly + * what a non-canonical cbm_detect_self_path vs the hardcoded target can + * produce) must also be detected as same-file and skipped, not truncated. */ + char link[512]; + snprintf(link, sizeof(link), "%s/self-link", tmpdir); + if (symlink(path, link) == 0) { + rc = cbm_copy_binary_to_target(link, path); + ASSERT_EQ(rc, 0); + data = read_test_file(path); + ASSERT_STR_EQ(data, "must survive self-copy"); /* still intact via symlink */ + } +#endif + + test_rmdir_r(tmpdir); + PASS(); +} + /* ═══════════════════════════════════════════════════════════════════ * Tar.gz extraction tests (port of update_test.go) * ═══════════════════════════════════════════════════════════════════ */ @@ -2654,6 +2730,10 @@ SUITE(cli) { /* Full lifecycle (1 test — cli_test.go) */ RUN_TEST(cli_install_and_uninstall); + /* Binary swap on install --force (#472) */ + RUN_TEST(cli_install_copies_binary_to_target_issue472); + RUN_TEST(cli_install_same_file_guard_issue472); + /* YAML parser (7 unit tests) */ RUN_TEST(cli_yaml_parse_simple); RUN_TEST(cli_yaml_parse_nested);