Merge pull request #2119 from DeusData/fix/2117-windows-user-path-uninstall

fix(cli): remove the install dir from the Windows user PATH on uninstall (#2117)

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);

tests/test_cli.c

@@ -45,6 +45,7 @@
 #endif
 #include <errno.h>
 #include <limits.h>
+#include <wchar.h> /* wcscmp/wcslen/swprintf for the Windows user-PATH test (#2117) */
 #include <zlib.h>
 
 /* 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) */

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:"):