diff --git a/packages/server/uninstall.test.ts b/packages/server/uninstall.test.ts index 696da6545..13f144f87 100644 --- a/packages/server/uninstall.test.ts +++ b/packages/server/uninstall.test.ts @@ -21,6 +21,8 @@ import { formatPurgeWarning, runPlannotatorUninstall, type UninstallEnvironment, + WINDOWS_PATH_RESTORE_SCRIPT, + WINDOWS_PATH_SCRIPT, WINDOWS_SELF_DELETE_SCRIPT, } from "./uninstall"; @@ -78,6 +80,95 @@ describe("Windows self-delete worker", () => { }); }); +describe("Windows PATH scripts", () => { + const scripts = { + remove: WINDOWS_PATH_SCRIPT, + restore: WINDOWS_PATH_RESTORE_SCRIPT, + } as const; + + test("edit the registry directly and never call the blocking .NET setter", () => { + // SetEnvironmentVariable('Path', ..., 'User') broadcasts WM_SETTINGCHANGE + // synchronously to every window and can stall past the 15 s command + // timeout on a machine with a hung GUI process (the CI smoke flake). + for (const script of Object.values(scripts)) { + expect(script).not.toContain("SetEnvironmentVariable"); + expect(script).toContain("Microsoft.Win32.Registry"); + // The broadcast that replaces it must be bounded (SMTO_ABORTIFHUNG) and + // must not be able to reach the exit code. + expect(script).toContain("SendMessageTimeout"); + expect(script).toMatch(/'Environment',0x2,\d+,\[ref\]\$r\)\}catch\{\}; exit 0$/); + } + // The value is read unexpanded and written back with its own kind so + // %VARS% in unrelated entries survive. + expect(scripts.remove).toContain("DoNotExpandEnvironmentNames"); + expect(scripts.remove).toContain("$k.SetValue('Path',$n,$kind)"); + // The completed-write echo must come after the write and before the + // broadcast in BOTH scripts: it is what lets the caller trust a write + // whose process was killed or faulted while broadcasting. + const echoes = { + remove: "Write-Output (ConvertTo-Json", + restore: "Write-Output 'PLANNOTATOR_PATH_RESTORED'", + } as const; + for (const [name, script] of Object.entries(scripts) as Array< + [keyof typeof scripts, string] + >) { + const writeIndex = script.indexOf("$k.SetValue("); + const echoIndex = script.indexOf(echoes[name]); + const broadcastIndex = script.indexOf("SendMessageTimeout("); + expect(`${name}: ${writeIndex}`).not.toBe(`${name}: -1`); + expect(echoIndex).toBeGreaterThan(writeIndex); + expect(broadcastIndex).toBeGreaterThan(echoIndex); + } + }); + + test("stay single-quoted so they survive -Command argv quoting", () => { + // Both scripts travel as one argv element to powershell.exe; a literal + // double quote would be re-escaped by the spawn layer and break parsing. + for (const script of Object.values(scripts)) { + expect(script).not.toContain('"'); + } + }); + + const powershell = + Bun.which("pwsh") || + Bun.which("pwsh.exe") || + Bun.which("powershell.exe") || + process.env.PLANNOTATOR_TEST_POWERSHELL || + null; + + test.skipIf(!powershell)( + "parse cleanly in a real PowerShell (no registry access)", + async () => { + // Parser.ParseInput only parses; nothing is executed, so this touches + // neither the registry nor the environment. Runs on Windows CI and on + // any dev box with pwsh. + for (const [name, script] of Object.entries(scripts)) { + const proc = Bun.spawn( + [ + powershell!, + "-NoProfile", + "-NonInteractive", + "-Command", + "$errors=$null; [void][System.Management.Automation.Language.Parser]::ParseInput($env:PLANNOTATOR_TEST_SCRIPT,[ref]$null,[ref]$errors); if($errors.Count -gt 0){$errors | ForEach-Object { Write-Output $_.Message }; exit 1}; exit 0", + ], + { + stdin: "ignore", + stdout: "pipe", + stderr: "pipe", + env: { ...process.env, PLANNOTATOR_TEST_SCRIPT: script }, + }, + ); + const [stdout, exitCode] = await Promise.all([ + new Response(proc.stdout).text(), + proc.exited, + ]); + expect(`${name}: ${stdout.trim()}`).toBe(`${name}: `); + expect(exitCode).toBe(0); + } + }, + ); +}); + function createFixture( overrides: Partial = {}, ): Fixture { @@ -1541,12 +1632,231 @@ describe("host and platform integrations", () => { expect(result.ok).toBe(false); expect(result.errors).toContain( - `Could not remove ${dirname(currentExe)} from the Windows user PATH.`, + `Could not remove ${dirname(currentExe)} from the Windows user PATH (exit 1).`, + ); + expect(existsSync(currentExe)).toBe(true); + expect(fixture.scheduledDeletes).toEqual([]); + }); + + test("proceeds without a PATH error when the entry is not present (exit 3)", async () => { + const fixture = createFixture(); + const localAppData = join(fixture.homeDir, "AppData", "Local"); + const currentExe = join(localAppData, "plannotator", "plannotator.exe"); + writeText(currentExe); + + const result = await runPlannotatorUninstall( + { purge: false, dryRun: false }, + { + ...fixture.environment, + platform: "win32", + execPath: currentExe, + env: { LOCALAPPDATA: localAppData }, + which: () => "C:\\Windows\\powershell.exe", + runCommand: async () => ({ exitCode: 3, timedOut: false }), + }, + ); + + expect(result.ok).toBe(true); + expect(result.errors).toEqual([]); + expect(result.removed).not.toContain( + `Windows user PATH entry ${dirname(currentExe)}`, + ); + expect(fixture.scheduledDeletes).toEqual([ + { target: currentExe, parent: dirname(currentExe) }, + ]); + }); + + test("reports a timed-out PATH edit as a timeout and keeps the CLI", async () => { + const fixture = createFixture(); + const localAppData = join(fixture.homeDir, "AppData", "Local"); + const currentExe = join(localAppData, "plannotator", "plannotator.exe"); + writeText(currentExe); + + const result = await runPlannotatorUninstall( + { purge: false, dryRun: false }, + { + ...fixture.environment, + platform: "win32", + execPath: currentExe, + env: { LOCALAPPDATA: localAppData }, + which: () => "C:\\Windows\\powershell.exe", + // Killed before the script echoed anything: the edit is unproven. + runCommand: async () => ({ exitCode: 124, timedOut: true, stdout: "" }), + }, + ); + + expect(result.ok).toBe(false); + expect(result.errors).toContain( + `Could not remove ${dirname(currentExe)} from the Windows user PATH (command timed out).`, ); expect(existsSync(currentExe)).toBe(true); expect(fixture.scheduledDeletes).toEqual([]); }); + test("treats a timeout after the rollback echo as a completed PATH edit", async () => { + const fixture = createFixture(); + const localAppData = join(fixture.homeDir, "AppData", "Local"); + const currentExe = join(localAppData, "plannotator", "plannotator.exe"); + writeText(currentExe); + const originalPath = `C:\\Before;${dirname(currentExe)};C:\\After;;`; + + const result = await runPlannotatorUninstall( + { purge: false, dryRun: false }, + { + ...fixture.environment, + platform: "win32", + execPath: currentExe, + env: { LOCALAPPDATA: localAppData }, + which: () => "C:\\Windows\\powershell.exe", + // The script echoes the original PATH only after the registry write, + // so an echo followed by a kill means only the broadcast stalled. + runCommand: async () => ({ + exitCode: 124, + timedOut: true, + stdout: `${JSON.stringify(originalPath)}\n`, + }), + }, + ); + + const pathLabel = `Windows user PATH entry ${dirname(currentExe)}`; + expect(result.ok).toBe(true); + expect(result.removed).toContain(pathLabel); + expect(result.warnings.some((w) => w.includes("timed out"))).toBe(true); + expect(fixture.scheduledDeletes).toEqual([ + { target: currentExe, parent: dirname(currentExe) }, + ]); + }); + + test("treats a non-zero exit after the rollback echo as a completed PATH edit", async () => { + const fixture = createFixture(); + const localAppData = join(fixture.homeDir, "AppData", "Local"); + const currentExe = join(localAppData, "plannotator", "plannotator.exe"); + writeText(currentExe); + const originalPath = `C:\\Before;${dirname(currentExe)};C:\\After;;`; + + const result = await runPlannotatorUninstall( + { purge: false, dryRun: false }, + { + ...fixture.environment, + platform: "win32", + execPath: currentExe, + env: { LOCALAPPDATA: localAppData }, + which: () => "C:\\Windows\\powershell.exe", + // A native fault inside Add-Type / SendMessageTimeout is not + // catchable and ends the process with an NTSTATUS code; the echo + // already on stdout still proves the write completed. + runCommand: async () => ({ + exitCode: -1073741819, + timedOut: false, + stdout: `${JSON.stringify(originalPath)}\n`, + }), + }, + ); + + const pathLabel = `Windows user PATH entry ${dirname(currentExe)}`; + expect(result.ok).toBe(true); + expect(result.errors).toEqual([]); + expect(result.removed).toContain(pathLabel); + expect(result.warnings.some((w) => w.includes("exit -1073741819"))).toBe(true); + expect(fixture.scheduledDeletes).toEqual([ + { target: currentExe, parent: dirname(currentExe) }, + ]); + }); + + test("treats a restore that printed its sentinel as completed however the process ended", async () => { + for (const ending of [ + { exitCode: 124, timedOut: true, needle: "timed out" }, + { exitCode: -1073741819, timedOut: false, needle: "exit -1073741819" }, + ]) { + const fixture = createFixture(); + const localAppData = join(fixture.homeDir, "AppData", "Local"); + const currentExe = join(localAppData, "plannotator", "plannotator.exe"); + writeText(currentExe); + let commandCount = 0; + + const result = await runPlannotatorUninstall( + { purge: false, dryRun: false }, + { + ...fixture.environment, + platform: "win32", + execPath: currentExe, + env: { LOCALAPPDATA: localAppData }, + which: () => "C:\\Windows\\powershell.exe", + runCommand: async () => { + commandCount += 1; + if (commandCount === 1) { + return { + exitCode: 0, + timedOut: false, + stdout: JSON.stringify( + `C:\\Before;${dirname(currentExe)};C:\\After;;`, + ), + }; + } + return { + exitCode: ending.exitCode, + timedOut: ending.timedOut, + stdout: "PLANNOTATOR_PATH_RESTORED\r\n", + }; + }, + scheduleWindowsSelfDelete: async () => false, + }, + ); + + const pathLabel = `Windows user PATH entry ${dirname(currentExe)}`; + expect(result.ok).toBe(false); + expect(existsSync(currentExe)).toBe(true); + expect(result.errors.some((e) => e.includes("Could not restore"))).toBe(false); + expect(result.removed).not.toContain(pathLabel); + expect(result.preserved).toContain(`${pathLabel} (restored for retry)`); + expect( + result.warnings.some( + (w) => w.startsWith("Restored ") && w.includes(ending.needle), + ), + ).toBe(true); + } + }); + + test("reports a restore that never printed its sentinel as failed", async () => { + const fixture = createFixture(); + const localAppData = join(fixture.homeDir, "AppData", "Local"); + const currentExe = join(localAppData, "plannotator", "plannotator.exe"); + writeText(currentExe); + let commandCount = 0; + + const result = await runPlannotatorUninstall( + { purge: false, dryRun: false }, + { + ...fixture.environment, + platform: "win32", + execPath: currentExe, + env: { LOCALAPPDATA: localAppData }, + which: () => "C:\\Windows\\powershell.exe", + runCommand: async () => { + commandCount += 1; + if (commandCount === 1) { + return { + exitCode: 0, + timedOut: false, + stdout: JSON.stringify( + `C:\\Before;${dirname(currentExe)};C:\\After;;`, + ), + }; + } + return { exitCode: 124, timedOut: true, stdout: "" }; + }, + scheduleWindowsSelfDelete: async () => false, + }, + ); + + const pathLabel = `Windows user PATH entry ${dirname(currentExe)}`; + expect(result.ok).toBe(false); + expect(result.removed).toContain(pathLabel); + expect(result.errors).toContain( + `Could not restore ${dirname(currentExe)} to the Windows user PATH after self-delete scheduling failed (command timed out).`, + ); + }); + test("restores Windows PATH when scheduling self-delete fails", async () => { const fixture = createFixture(); const localAppData = join(fixture.homeDir, "AppData", "Local"); @@ -1615,7 +1925,7 @@ describe("host and platform integrations", () => { expect(existsSync(currentExe)).toBe(true); expect(result.removed).toContain(pathLabel); expect(result.errors).toContain( - `Could not restore ${dirname(currentExe)} to the Windows user PATH after self-delete scheduling failed.`, + `Could not restore ${dirname(currentExe)} to the Windows user PATH after self-delete scheduling failed (exit 1).`, ); expect(result.warnings).toContain( `The Plannotator CLI remains at ${currentExe}, but its Windows PATH entry could not be restored. Run that full path to retry, then restore PATH manually if needed.`, diff --git a/packages/server/uninstall.ts b/packages/server/uninstall.ts index 8a8f6af16..46193c553 100644 --- a/packages/server/uninstall.ts +++ b/packages/server/uninstall.ts @@ -108,22 +108,84 @@ const PURGE_OWNED_TOP_LEVEL = [ "guide-schema.json", ] as const; -const WINDOWS_PATH_SCRIPT = [ +/** + * Best-effort WM_SETTINGCHANGE broadcast that runs AFTER the registry write. + * + * `[Environment]::SetEnvironmentVariable(..., 'User')` performs this broadcast + * itself, synchronously, per top-level window and without SMTO_ABORTIFHUNG, so + * one orphaned or hung GUI process can stall it past the uninstaller's 15 s + * command timeout and turn a completed PATH edit into a killed PowerShell and + * exit 124. Here the broadcast is decoupled from the edit: SMTO_ABORTIFHUNG + * (0x2) skips windows Windows already considers hung, the per-window timeout + * is short, the whole thing is wrapped in try/catch, and the script exits 0 + * explicitly so nothing about the broadcast can reach the exit code. + * + * The DllImport attribute needs double quotes, which the `-Command` one-liners + * avoid on purpose (the script travels as a single argv element on Windows), + * so they are assembled from `[char]34` at runtime. + */ +const WINDOWS_PATH_BROADCAST_STATEMENTS = [ + "$q=[char]34", + "$sig='[DllImport('+$q+'user32.dll'+$q+',CharSet=CharSet.Unicode)] public static extern IntPtr SendMessageTimeout(IntPtr hWnd,uint msg,UIntPtr wParam,string lParam,uint flags,uint timeout,out UIntPtr result);'", + "try{Add-Type -Namespace Plannotator -Name PathBroadcast -MemberDefinition $sig; $r=[UIntPtr]::Zero; [void][Plannotator.PathBroadcast]::SendMessageTimeout([IntPtr]0xffff,0x1A,[UIntPtr]::Zero,'Environment',0x2,1000,[ref]$r)}catch{}", + "exit 0", +] as const; + +/** + * Removes exactly one entry from the HKCU user PATH through the registry API. + * + * The value is read unexpanded (`DoNotExpandEnvironmentNames`) and written back + * with its original kind (REG_EXPAND_SZ on most systems), so `%VARS%` in other + * entries survive byte for byte. Exit 3 means "not present or unchanged"; on + * success the ORIGINAL value is echoed as one JSON string on stdout so the + * caller can roll back. The echo is written before the broadcast, so stdout + * carrying that JSON proves the registry write completed. + * + * @internal Exported only so the PowerShell syntax can be regression-tested. + */ +export const WINDOWS_PATH_SCRIPT = [ "$ErrorActionPreference='Stop'", - "$p=[Environment]::GetEnvironmentVariable('Path','User')", + "$k=[Microsoft.Win32.Registry]::CurrentUser.OpenSubKey('Environment',$true)", + "if($null -eq $k){exit 3}", + "$p=$k.GetValue('Path',$null,[Microsoft.Win32.RegistryValueOptions]::DoNotExpandEnvironmentNames)", "if($null -eq $p){exit 3}", + "$p=[string]$p", + "$kind=$k.GetValueKind('Path')", "$t=$env:PLANNOTATOR_UNINSTALL_PATH.Trim().TrimEnd('\\')", "$kept=@($p -split ';' | Where-Object { $_.Trim().TrimEnd('\\') -ine $t })", "$n=$kept -join ';'", "if($n -eq $p){exit 3}", - "[Environment]::SetEnvironmentVariable('Path',$n,'User')", + "$k.SetValue('Path',$n,$kind)", + "$k.Close()", "Write-Output (ConvertTo-Json -Compress -InputObject $p)", + ...WINDOWS_PATH_BROADCAST_STATEMENTS, ].join("; "); -const WINDOWS_PATH_RESTORE_SCRIPT = [ +/** + * Line the restore script prints right after its registry write and before the + * broadcast, so the caller can tell a completed restore from one that never + * reached the write even when the process was killed or died afterwards. + */ +const WINDOWS_PATH_RESTORED_SENTINEL = "PLANNOTATOR_PATH_RESTORED"; + +/** + * Writes the echoed original PATH back with the kind the value currently has + * (the kind the removal preserved), falling back to REG_EXPAND_SZ when the + * value is gone entirely. Same decoupled broadcast as the removal. + * + * @internal Exported only so the PowerShell syntax can be regression-tested. + */ +export const WINDOWS_PATH_RESTORE_SCRIPT = [ "$ErrorActionPreference='Stop'", "$original=$env:PLANNOTATOR_UNINSTALL_ORIGINAL_PATH", - "[Environment]::SetEnvironmentVariable('Path',$original,'User')", + "$k=[Microsoft.Win32.Registry]::CurrentUser.OpenSubKey('Environment',$true)", + "if($null -eq $k){$k=[Microsoft.Win32.Registry]::CurrentUser.CreateSubKey('Environment')}", + "$kind=[Microsoft.Win32.RegistryValueKind]::ExpandString", + "try{$kind=$k.GetValueKind('Path')}catch{}", + "$k.SetValue('Path',$original,$kind)", + "$k.Close()", + `Write-Output '${WINDOWS_PATH_RESTORED_SENTINEL}'`, + ...WINDOWS_PATH_BROADCAST_STATEMENTS, ].join("; "); /** @internal Exported only so the Windows worker syntax can be regression-tested. */ @@ -888,28 +950,43 @@ async function removeWindowsPathEntry( ["-NoProfile", "-NonInteractive", "-Command", WINDOWS_PATH_SCRIPT], { PLANNOTATOR_UNINSTALL_PATH: paths.windowsInstallDir }, ); - if (result.exitCode === 0) { + // The script echoes the original PATH only after the registry write + // succeeded, so a parseable echo proves the edit happened however the + // process ended afterwards: killed by the timeout while broadcasting, or + // taken down by a native fault inside Add-Type / SendMessageTimeout that no + // try/catch can intercept and that exits with an NTSTATUS code. + const echoedOriginalPath = parseEchoedWindowsPath(result.stdout); + if (result.exitCode === 0 || echoedOriginalPath !== null) { state.removed.push(label); - try { - const originalPath: unknown = JSON.parse(result.stdout?.trim() ?? ""); - if (typeof originalPath !== "string") throw new Error("not a string"); - return originalPath; - } catch { - state.errors.push( - `Removed ${paths.windowsInstallDir} from the Windows user PATH but could not capture the original PATH for safe rollback.`, - ); + if (result.exitCode !== 0) { state.warnings.push( - `The Plannotator CLI remains at ${environment.execPath}, but its Windows PATH entry was removed without a usable backup. Run that full path to retry, then restore PATH manually if needed.`, + `Removed ${paths.windowsInstallDir} from the Windows user PATH, but notifying open windows of the change ${result.timedOut ? "timed out" : `failed (exit ${result.exitCode})`}; new terminals pick up the change after you sign in again.`, ); } + if (echoedOriginalPath !== null) return echoedOriginalPath; + state.errors.push( + `Removed ${paths.windowsInstallDir} from the Windows user PATH but could not capture the original PATH for safe rollback.`, + ); + state.warnings.push( + `The Plannotator CLI remains at ${environment.execPath}, but its Windows PATH entry was removed without a usable backup. Run that full path to retry, then restore PATH manually if needed.`, + ); } else if (result.exitCode !== 3) { state.errors.push( - `Could not remove ${paths.windowsInstallDir} from the Windows user PATH.`, + `Could not remove ${paths.windowsInstallDir} from the Windows user PATH (${result.timedOut ? "command timed out" : `exit ${result.exitCode}`}).`, ); } return null; } +function parseEchoedWindowsPath(stdout: string | undefined): string | null { + try { + const originalPath: unknown = JSON.parse(stdout?.trim() ?? ""); + return typeof originalPath === "string" ? originalPath : null; + } catch { + return null; + } +} + async function removeBinaries( request: UninstallRequest, environment: UninstallEnvironment, @@ -998,15 +1075,26 @@ async function restoreWindowsPathEntry( ["-NoProfile", "-NonInteractive", "-Command", WINDOWS_PATH_RESTORE_SCRIPT], { PLANNOTATOR_UNINSTALL_ORIGINAL_PATH: originalPath }, ); - if (result.exitCode !== 0) { + // Same proof as the removal: the sentinel is printed only after the + // registry write, so its presence means the restore completed even if the + // broadcast then timed out or faulted. + const restored = (result.stdout ?? "") + .split(/\r?\n/) + .some((line) => line.trim() === WINDOWS_PATH_RESTORED_SENTINEL); + if (result.exitCode !== 0 && !restored) { state.errors.push( - `Could not restore ${paths.windowsInstallDir} to the Windows user PATH after self-delete scheduling failed.`, + `Could not restore ${paths.windowsInstallDir} to the Windows user PATH after self-delete scheduling failed (${result.timedOut ? "command timed out" : `exit ${result.exitCode}`}).`, ); state.warnings.push( `The Plannotator CLI remains at ${environment.execPath}, but its Windows PATH entry could not be restored. Run that full path to retry, then restore PATH manually if needed.`, ); return; } + if (result.exitCode !== 0) { + state.warnings.push( + `Restored ${paths.windowsInstallDir} to the Windows user PATH, but notifying open windows of the change ${result.timedOut ? "timed out" : `failed (exit ${result.exitCode})`}; new terminals pick up the change after you sign in again.`, + ); + } const label = `Windows user PATH entry ${paths.windowsInstallDir}`; const removedIndex = state.removed.indexOf(label);