From 8135be28a764fad4ee55abe466762bbda7ebab74 Mon Sep 17 00:00:00 2001 From: Martin Vogel Date: Tue, 8 Sep 2026 22:08:39 +0200 Subject: [PATCH] fix(cli): remove the install dir from the Windows user PATH on uninstall (#2117) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On Windows, install registers the install directory in the persistent HKCU\Environment\Path, but uninstall never removed it — so every install/uninstall cycle left its entry behind and the user PATH grew without bound (a real test host reached ~8000 chars, emptying the effective PATH and breaking tooling). The reported non-ASCII "mojibake" is historical, not a current-code bug. Both install paths already write the value through the UTF-16 registry API: install.ps1 (shipped in each release archive) runs `cbm install`, which calls RegSetValueExW(L"Path", ...) — git history shows no ANSI (RegSetValueExA / SetEnvironmentVariableA) predecessor — and setup-windows.ps1 writes via .NET [Environment]::SetEnvironmentVariable (Unicode) with an ASCII-only install dir. The only path that handles a non-ASCII directory is the wide-correct C path, so current code cannot produce a garbled entry; the garbage the reporter saw was left by an older binary. No code change is needed for the encoding itself. The real defect is the missing uninstall-side removal. This adds cli_remove_windows_user_path(), the counterpart to cli_ensure_windows_user_path(): it opens the same key, walks the ';'-separated segments, and drops exactly the ones equal to the install dir using the same case- and trailing-separator-insensitive comparison the install `present` scan uses, so it removes precisely what install would have de-duplicated. Every other segment is preserved byte-for-byte, the value kind (REG_SZ / REG_EXPAND_SZ) is preserved, and a registry error is a warning rather than a hard failure so a user is never blocked from finishing an uninstall over a cosmetic PATH edit. Removing the entry on uninstall also stops accumulation on reinstall: a garbled historical entry defeats the install de-dup and gets re-added, so taking our own entry back out keeps the value bounded. Testing: - tests/test_cli.c: a hermetic Windows unit test drives the append and remove logic against a GUID-scoped scratch key via the existing CBM_TEST_WINDOWS_USER_PATH_RUN_ID seam, never the developer's live HKCU\Environment\Path. A non-ASCII install dir doubles as a mojibake guard (a broken UTF-8->UTF-16 round-trip would fail the match and leak a duplicate). It asserts append, idempotent reinstall, exact removal from the tail and the middle, idempotent removal, and byte-for-byte survival of unrelated entries. - tests/test_smoke_fixture_contract.sh: a macOS-runnable source contract that fails when the removal is absent and passes with it, proving the wiring binds (verified RED-on-revert). The Windows-only registry code is #ifdef _WIN32; its compile and the behavioral unit test are verified by CI's Windows leg (the local Windows VM is down). Pre-existing mojibaked entries from old binaries are a separate one-time cleanup, out of scope here, and setup-windows.ps1's own lack of an uninstall mode is noted as a possible follow-up. Fixes #2117 Co-Authored-By: Claude Fable 5.1 Signed-off-by: Martin Vogel --- src/cli/cli.c | 158 +++++++++++++++++++++++++++ tests/test_cli.c | 106 ++++++++++++++++++ tests/test_smoke_fixture_contract.sh | 7 ++ 3 files changed, 271 insertions(+) diff --git a/src/cli/cli.c b/src/cli/cli.c index d85ebf0b6..956489cd9 100644 --- a/src/cli/cli.c +++ b/src/cli/cli.c @@ -6491,6 +6491,127 @@ static int cli_ensure_windows_user_path(const char *bin_dir, bool dry_run) { return CLI_OK; } +/* Uninstall counterpart to cli_ensure_windows_user_path: remove exactly the + * install-dir segment install added, leaving every other segment byte-for-byte + * intact. Without this, every install/uninstall cycle leaves its entry behind + * and the current-user PATH grows without bound (#2117). Segment identity uses + * the same case- and trailing-separator-insensitive comparison as the install + * `present` scan, so we remove precisely what install would have de-duplicated. + * Returns CLI_OK when a segment was removed, CLI_TRUE when the directory was + * not present (nothing to do), CLI_ERR on a registry failure. dry_run reports + * without mutating. */ +static int cli_remove_windows_user_path(const char *bin_dir, bool dry_run) { + wchar_t *wide_dir = cli_windows_utf8_to_wide(bin_dir); + HKEY environment = NULL; + if (!wide_dir || cli_windows_open_user_path_key(&environment) != CLI_OK) { + free(wide_dir); + return CLI_ERR; + } + + DWORD type = REG_EXPAND_SZ; + DWORD bytes = 0; + LONG queried = RegQueryValueExW(environment, L"Path", NULL, &type, NULL, &bytes); + if (queried == ERROR_FILE_NOT_FOUND) { + /* No user PATH value at all — nothing of ours to remove. */ + RegCloseKey(environment); + free(wide_dir); + return CLI_TRUE; + } + if (queried != ERROR_SUCCESS || (type != REG_SZ && type != REG_EXPAND_SZ)) { + RegCloseKey(environment); + free(wide_dir); + return CLI_ERR; + } + size_t existing_capacity = (size_t)bytes / sizeof(wchar_t) + 1U; + wchar_t *existing = calloc(existing_capacity, sizeof(*existing)); + if (!existing) { + RegCloseKey(environment); + free(wide_dir); + return CLI_ERR; + } + DWORD read_bytes = bytes; + if (RegQueryValueExW(environment, L"Path", NULL, &type, (BYTE *)existing, &read_bytes) != + ERROR_SUCCESS) { + RegCloseKey(environment); + free(existing); + free(wide_dir); + return CLI_ERR; + } + existing[existing_capacity - 1U] = L'\0'; + + /* Rebuild the value from every segment that is NOT our directory. Kept + * segments keep their exact original characters; a single ';' rejoins + * consecutive kept segments so unrelated entries survive byte-for-byte and + * no stray separator is left where our entry used to be. The result is + * never longer than the input, so the input length bounds the buffer. */ + size_t existing_length = wcslen(existing); + wchar_t *rebuilt = calloc(existing_length + 1U, sizeof(*rebuilt)); + if (!rebuilt) { + RegCloseKey(environment); + free(existing); + free(wide_dir); + return CLI_ERR; + } + size_t out = 0; + bool removed = false; + bool wrote_segment = false; + const wchar_t *cursor = existing; + while (*cursor) { + const wchar_t *separator = wcschr(cursor, L';'); + size_t length = separator ? (size_t)(separator - cursor) : wcslen(cursor); + if (cli_windows_path_segment_equal(cursor, length, wide_dir)) { + removed = true; + } else { + if (wrote_segment) { + rebuilt[out++] = L';'; + } + memcpy(rebuilt + out, cursor, length * sizeof(*rebuilt)); + out += length; + wrote_segment = true; + } + cursor = separator ? separator + 1 : cursor + length; + } + rebuilt[out] = L'\0'; + + if (!removed) { + RegCloseKey(environment); + free(rebuilt); + free(existing); + free(wide_dir); + return CLI_TRUE; + } + if (dry_run) { + RegCloseKey(environment); + free(rebuilt); + free(existing); + free(wide_dir); + return CLI_OK; + } + DWORD output_bytes = (DWORD)((out + 1U) * sizeof(*rebuilt)); + LONG stored = + RegSetValueExW(environment, L"Path", 0, type, (const BYTE *)rebuilt, output_bytes); + RegCloseKey(environment); + free(rebuilt); + free(existing); + free(wide_dir); + if (stored != ERROR_SUCCESS) { + return CLI_ERR; + } + return CLI_OK; +} + +#if defined(CBM_CLI_ENABLE_TEST_API) +/* Thin seams so the hermetic Windows PATH unit test can drive the append and + * remove logic against a GUID-scoped scratch key (the CBM_TEST_WINDOWS_USER_ + * PATH_RUN_ID seam) without touching the developer's live HKCU PATH. */ +int cbm_cli_ensure_windows_user_path_for_test(const char *bin_dir, bool dry_run) { + return cli_ensure_windows_user_path(bin_dir, dry_run); +} +int cbm_cli_remove_windows_user_path_for_test(const char *bin_dir, bool dry_run) { + return cli_remove_windows_user_path(bin_dir, dry_run); +} +#endif + #endif /* ── Tar.gz / zip extraction (TEST-ONLY) ────────────────────────── @@ -12081,6 +12202,43 @@ static int cli_uninstall_activate(void *opaque) { return CLI_ACTIVATION_PARTIAL; } +#ifdef _WIN32 + /* #2117: install registers the install directory in the persistent + * current-user PATH; uninstall must take it back out, or every cycle leaves + * a stale entry and the PATH grows without bound. Remove only our segment. + * A registry hiccup here is a warning, never a hard failure: the user is + * removing the tool and must not be blocked from finishing over a cosmetic + * PATH edit. Suppress the mutation under the test-ops seam exactly as + * install does, so the CLI suite never touches the developer's real PATH. */ + if (activation->bin_path && activation->bin_path[0]) { + const char *slash = strrchr(activation->bin_path, '/'); + const char *backslash = strrchr(activation->bin_path, '\\'); + if (backslash && (!slash || backslash > slash)) { + slash = backslash; + } + if (slash && slash != activation->bin_path) { + size_t dir_len = (size_t)(slash - activation->bin_path); + char bin_dir[CLI_BUF_1K]; + if (dir_len < sizeof(bin_dir)) { + memcpy(bin_dir, activation->bin_path, dir_len); + bin_dir[dir_len] = '\0'; + int path_rc = cli_remove_windows_user_path( + bin_dir, activation->dry_run || g_cli_activation_test_ops_set); + if (path_rc == CLI_OK) { + printf(activation->dry_run ? "\nWould remove %s from the current-user PATH\n" + : "\nRemoved %s from the current-user PATH\n", + bin_dir); + } else if (path_rc == CLI_ERR) { + (void)fprintf(stderr, + "warning: could not update the current-user PATH; %s may " + "remain on it\n", + bin_dir); + } + } + } + } +#endif + if (activation->delete_indexes && !activation->dry_run) { int expected = count_db_indexes(activation->home); int idx_removed = cbm_remove_indexes(activation->home); diff --git a/tests/test_cli.c b/tests/test_cli.c index 976280868..ae1a5c5f4 100644 --- a/tests/test_cli.c +++ b/tests/test_cli.c @@ -45,6 +45,7 @@ #endif #include #include +#include /* wcscmp/wcslen/swprintf for the Windows user-PATH test (#2117) */ #include /* Internal prompt seam used to restore process-global state after command @@ -14670,6 +14671,110 @@ TEST(cli_windows_update_hands_off_to_install_script) { ASSERT_TRUE(preserved); PASS(); } + +/* #2117: install registers the install dir in the persistent current-user PATH + * via the wide registry API and de-duplicates on reinstall; uninstall must take + * exactly that entry back out and leave every other entry byte-for-byte intact, + * or the PATH grows without bound (a real Windows host reached ~8000 chars). + * This drives the actual query/append/remove logic through the hermetic + * CBM_TEST_WINDOWS_USER_PATH_RUN_ID seam, against a GUID-scoped scratch key + * under HKCU\Software\CodebaseMemoryMCP\Smoke — the developer's live + * HKCU\Environment\Path is never opened. The non-ASCII install dir doubles as + * the mojibake guard: a broken UTF-8->UTF-16 round-trip would fail the segment + * match and leak a duplicate instead of de-duplicating. + * + * Return codes mirror the internal enum: 0 = added/removed, 1 = already + * present / nothing to remove. */ +int cbm_cli_ensure_windows_user_path_for_test(const char *bin_dir, bool dry_run); +int cbm_cli_remove_windows_user_path_for_test(const char *bin_dir, bool dry_run); + +static const wchar_t k_cli_user_path_run_id[] = L"0123456789abcdef0123456789abcdef"; + +static int cli_test_read_smoke_path(HKEY key, wchar_t *out, DWORD out_bytes) { + DWORD type = 0; + DWORD bytes = out_bytes; + return RegQueryValueExW(key, L"Path", NULL, &type, (BYTE *)out, &bytes) == ERROR_SUCCESS ? 0 + : -1; +} + +static int cli_test_set_smoke_path(HKEY key, const wchar_t *value) { + DWORD bytes = (DWORD)((wcslen(value) + 1U) * sizeof(wchar_t)); + LONG rc = RegSetValueExW(key, L"Path", 0, REG_EXPAND_SZ, (const BYTE *)value, bytes); + return rc == ERROR_SUCCESS ? 0 : -1; +} + +TEST(cli_uninstall_removes_only_its_own_windows_user_path_entry_issue2117) { + if (!SetEnvironmentVariableW(L"CBM_TEST_WINDOWS_USER_PATH_RUN_ID", k_cli_user_path_run_id) || + !SetEnvironmentVariableW(L"SMOKE_TEMP_ROOT", L"C:\\Temp\\cbm-smoke") || + !SetEnvironmentVariableW(L"SMOKE_DOWNLOAD_URL", L"http://127.0.0.1:8765")) { + FAIL("could not arm the Windows user-PATH test seam"); + } + + wchar_t key_path[128]; + swprintf(key_path, sizeof(key_path) / sizeof(key_path[0]), + L"Software\\CodebaseMemoryMCP\\Smoke\\%ls", k_cli_user_path_run_id); + /* Start from a clean leaf so a prior aborted run cannot skew the result. */ + RegDeleteKeyW(HKEY_CURRENT_USER, key_path); + HKEY key = NULL; + if (RegCreateKeyExW(HKEY_CURRENT_USER, key_path, 0, NULL, REG_OPTION_NON_VOLATILE, + KEY_QUERY_VALUE | KEY_SET_VALUE | KEY_WOW64_64KEY, NULL, &key, + NULL) != ERROR_SUCCESS) { + FAIL("could not create the scratch smoke registry key"); + } + DWORD sentinel_bytes = (DWORD)((wcslen(k_cli_user_path_run_id) + 1U) * sizeof(wchar_t)); + RegSetValueExW(key, L"CbmSmokeRunId", 0, REG_SZ, (const BYTE *)k_cli_user_path_run_id, + sentinel_bytes); + + /* Non-ASCII install dir (Omega mu epsilon-tonos gamma alpha). */ + wchar_t bin_dir_w[] = L"C:\\Users\\dev\\install_\u03A9\u03BC\u03AD\u03B3\u03B1\\bin"; + char bin_dir[512]; + WideCharToMultiByte(CP_UTF8, 0, bin_dir_w, -1, bin_dir, (int)sizeof(bin_dir), NULL, NULL); + + wchar_t readback[1024]; + wchar_t expected_appended[1024]; + swprintf(expected_appended, sizeof(expected_appended) / sizeof(expected_appended[0]), + L"C:\\Tools\\alpha;C:\\Tools\\omega;%ls", bin_dir_w); + wchar_t seeded_middle[1024]; + swprintf(seeded_middle, sizeof(seeded_middle) / sizeof(seeded_middle[0]), + L"C:\\Tools\\alpha;%ls;C:\\Tools\\omega", bin_dir_w); + + int ok = 1; + /* Scenario A — append to a PATH lacking our dir, de-dup, then remove. */ + ok &= cli_test_set_smoke_path(key, L"C:\\Tools\\alpha;C:\\Tools\\omega") == 0; + ok &= cbm_cli_ensure_windows_user_path_for_test(bin_dir, false) == 0; /* added */ + ok &= cli_test_read_smoke_path(key, readback, sizeof(readback)) == 0; + ok &= wcscmp(readback, expected_appended) == 0; + /* Reinstall must not duplicate: the non-ASCII entry must round-trip-match. */ + ok &= cbm_cli_ensure_windows_user_path_for_test(bin_dir, false) == 1; /* already present */ + ok &= cli_test_read_smoke_path(key, readback, sizeof(readback)) == 0; + ok &= wcscmp(readback, expected_appended) == 0; + /* Remove our entry — the two unrelated entries survive byte-for-byte. */ + ok &= cbm_cli_remove_windows_user_path_for_test(bin_dir, false) == 0; /* removed */ + ok &= cli_test_read_smoke_path(key, readback, sizeof(readback)) == 0; + ok &= wcscmp(readback, L"C:\\Tools\\alpha;C:\\Tools\\omega") == 0; + /* Idempotent removal: nothing of ours left to take out. */ + ok &= cbm_cli_remove_windows_user_path_for_test(bin_dir, false) == 1; /* nothing to remove */ + ok &= cli_test_read_smoke_path(key, readback, sizeof(readback)) == 0; + ok &= wcscmp(readback, L"C:\\Tools\\alpha;C:\\Tools\\omega") == 0; + + /* Scenario B — our entry in the MIDDLE: neighbours on both sides survive. */ + ok &= cli_test_set_smoke_path(key, seeded_middle) == 0; + ok &= cbm_cli_remove_windows_user_path_for_test(bin_dir, false) == 0; /* removed */ + ok &= cli_test_read_smoke_path(key, readback, sizeof(readback)) == 0; + ok &= wcscmp(readback, L"C:\\Tools\\alpha;C:\\Tools\\omega") == 0; + + /* Teardown: delete only our scratch leaf; clear the seam env. */ + RegCloseKey(key); + RegDeleteKeyW(HKEY_CURRENT_USER, key_path); + SetEnvironmentVariableW(L"CBM_TEST_WINDOWS_USER_PATH_RUN_ID", NULL); + SetEnvironmentVariableW(L"SMOKE_TEMP_ROOT", NULL); + SetEnvironmentVariableW(L"SMOKE_DOWNLOAD_URL", NULL); + + if (!ok) { + FAIL("uninstall must remove exactly its own user-PATH entry and leave others intact"); + } + PASS(); +} #endif /* _WIN32 */ /* ═══════════════════════════════════════════════════════════════════ @@ -14796,6 +14901,7 @@ SUITE(cli) { RUN_TEST(cli_activation_guard_is_bypassed_for_dry_run_and_plan); #ifdef _WIN32 RUN_TEST(cli_windows_update_hands_off_to_install_script); + RUN_TEST(cli_uninstall_removes_only_its_own_windows_user_path_entry_issue2117); #endif /* Version (2 tests — selfupdate_test.go) */ diff --git a/tests/test_smoke_fixture_contract.sh b/tests/test_smoke_fixture_contract.sh index 2971326d0..eb59bfbfc 100755 --- a/tests/test_smoke_fixture_contract.sh +++ b/tests/test_smoke_fixture_contract.sh @@ -265,6 +265,13 @@ require( and "CbmSmokeRunId" in cli_source, "the Windows PATH test seam must be run-ID-only, sentinel-bound, and loopback-gated", ) +require( + "cli_remove_windows_user_path" in cli_source + and "Removed %s from the current-user PATH" in cli_source + and "Would remove %s from the current-user PATH" in cli_source, + "uninstall must remove exactly its own segment from the current-user PATH (#2117), " + "or repeated install/uninstall cycles grow the User PATH without bound", +) # Every maintained local/PR native leg enters through the fixture wrapper. for service in ("smoke:", "smoke-amd64:"):