From a80f14db8c4332f5ef0bb609a09b12dbd2351bdd Mon Sep 17 00:00:00 2001 From: Evgeniy Podivilov Date: Sat, 20 Jun 2026 21:39:35 +0100 Subject: [PATCH 1/2] chore(self-update): surface Windows hint and warn on xattr failure MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Detect Windows in detectBinaryName and return a message that points users at install.ps1 instead of the generic "Unsupported platform" fallback, which gives no clue that Windows is actually supported via the install script. Extract the macOS quarantine removal into a small, injectable helper. The previous implementation swallowed every failure silently, leaving the user without any feedback when xattr was missing or errored — and without any hint why Gatekeeper might still block the new binary. Failures now surface as ui.warn() without failing the update, since quarantine removal is best-effort. Add unit tests for the Windows branch, the generic fallback, and the quarantine warn-on-failure path. --- src/cli/commands/self-update.test.ts | 104 +++++++++++++++++++++++++++ src/cli/commands/self-update.ts | 87 ++++++++++++++++------ 2 files changed, 170 insertions(+), 21 deletions(-) create mode 100644 src/cli/commands/self-update.test.ts diff --git a/src/cli/commands/self-update.test.ts b/src/cli/commands/self-update.test.ts new file mode 100644 index 0000000..7dac730 --- /dev/null +++ b/src/cli/commands/self-update.test.ts @@ -0,0 +1,104 @@ +import { describe, expect, test } from "bun:test"; +import { Result as R } from "../../shared/result.ts"; +import { + detectBinaryName, + type QuarantineRemover, + tryRemoveMacosQuarantine, + WINDOWS_UNSUPPORTED_MESSAGE, +} from "./self-update.ts"; + +describe("detectBinaryName", () => { + test("win32 → returns Windows-specific error pointing at install.ps1", () => { + const result = detectBinaryName("win32", "x64"); + expect(R.isErr(result)).toBe(true); + if (R.isErr(result)) { + expect(result.error.message).toBe(WINDOWS_UNSUPPORTED_MESSAGE); + } + }); + + test("win32 message does not look like the generic unsupported-platform fallback", () => { + const result = detectBinaryName("win32", "x64"); + expect(R.isErr(result)).toBe(true); + if (R.isErr(result)) { + expect(result.error.message).not.toMatch(/^Unsupported platform:/); + expect(result.error.message).toContain("install.ps1"); + } + }); + + test("other unsupported platforms keep the generic message", () => { + // freebsd is not in the supported set and is not win32, so it must fall through + // to the generic "Unsupported platform: …" branch. + const result = detectBinaryName("freebsd" as NodeJS.Platform, "x64"); + expect(R.isErr(result)).toBe(true); + if (R.isErr(result)) { + expect(result.error.message).toBe("Unsupported platform: freebsd/x64"); + } + }); + + test("unsupported arch on a supported os keeps the generic message", () => { + const result = detectBinaryName("linux", "ia32"); + expect(R.isErr(result)).toBe(true); + if (R.isErr(result)) { + expect(result.error.message).toBe("Unsupported platform: linux/ia32"); + } + }); + + test("darwin/arm64 → wt-darwin-arm64", () => { + const result = detectBinaryName("darwin", "arm64"); + expect(R.isOk(result)).toBe(true); + if (R.isOk(result)) { + expect(result.data).toBe("wt-darwin-arm64"); + } + }); + + test("linux/x64 → wt-linux-x64", () => { + const result = detectBinaryName("linux", "x64"); + expect(R.isOk(result)).toBe(true); + if (R.isOk(result)) { + expect(result.data).toBe("wt-linux-x64"); + } + }); +}); + +describe("tryRemoveMacosQuarantine", () => { + test("remover ok → no warning", () => { + const warnings: string[] = []; + const remover: QuarantineRemover = () => R.ok(undefined); + + tryRemoveMacosQuarantine("/path/to/wt", { + remover, + warn: (m) => warnings.push(m), + }); + + expect(warnings).toEqual([]); + }); + + test("remover errs → warning logged, no throw", () => { + const warnings: string[] = []; + const remover: QuarantineRemover = () => R.err(new Error("xattr: command not found")); + + expect(() => + tryRemoveMacosQuarantine("/path/to/wt", { + remover, + warn: (m) => warnings.push(m), + }), + ).not.toThrow(); + + expect(warnings).toHaveLength(1); + expect(warnings[0]).toContain("xattr: command not found"); + expect(warnings[0]).toContain("/path/to/wt"); + }); + + test("remover errs with non-Error → message still surfaces", () => { + const warnings: string[] = []; + const remover: QuarantineRemover = () => R.err(new Error("exited with code 1")); + + tryRemoveMacosQuarantine("/usr/local/bin/wt", { + remover, + warn: (m) => warnings.push(m), + }); + + expect(warnings[0]).toContain("exited with code 1"); + expect(warnings[0]).toContain("/usr/local/bin/wt"); + }); +}); diff --git a/src/cli/commands/self-update.ts b/src/cli/commands/self-update.ts index bd8ce43..b7ea4a8 100644 --- a/src/cli/commands/self-update.ts +++ b/src/cli/commands/self-update.ts @@ -13,9 +13,12 @@ import { UPDATE_CHECK_FILENAME } from "../update-notifier.ts"; const REPO = "epodivilov/worktree-kit"; -function detectBinaryName(): Result { - const platform = process.platform; - const arch = process.arch; +export const WINDOWS_UNSUPPORTED_MESSAGE = "Windows is not supported by self-update; reinstall via install.ps1"; + +export function detectBinaryName(platform: NodeJS.Platform, arch: string): Result { + if (platform === "win32") { + return R.err(new Error(WINDOWS_UNSUPPORTED_MESSAGE)); + } const os = platform === "darwin" ? "darwin" : platform === "linux" ? "linux" : null; const cpu = arch === "arm64" ? "arm64" : arch === "x64" ? "x64" : null; @@ -27,16 +30,57 @@ function detectBinaryName(): Result { return R.ok(`wt-${os}-${cpu}`); } +/** + * Best-effort removal of the macOS quarantine attribute from a freshly + * downloaded binary. Failures (missing `xattr`, non-zero exit, or no + * attribute present) must NOT fail the self-update — they only surface + * as a warning so the user knows why the binary might be Gatekeeper-blocked. + */ +export type QuarantineRemover = (targetPath: string) => Result; + +export const defaultQuarantineRemover: QuarantineRemover = (targetPath) => { + try { + const proc = Bun.spawnSync(["xattr", "-d", "com.apple.quarantine", targetPath]); + if (proc.exitCode !== 0) { + const stderr = proc.stderr?.toString().trim(); + return R.err(new Error(stderr || `xattr exited with code ${proc.exitCode}`)); + } + return R.ok(undefined); + } catch (err) { + return R.err(err instanceof Error ? err : new Error(String(err))); + } +}; + +export function tryRemoveMacosQuarantine( + targetPath: string, + deps: { remover: QuarantineRemover; warn: (message: string) => void }, +): void { + const result = deps.remover(targetPath); + if (R.isErr(result)) { + deps.warn( + `Could not remove macOS quarantine attribute (${result.error.message}); macOS Gatekeeper may block the new binary until you run \`xattr -d com.apple.quarantine ${targetPath}\` manually.`, + ); + } +} + function formatMb(bytes: number): string { return (bytes / 1024 / 1024).toFixed(1); } +interface DownloadBinaryDeps { + platform: NodeJS.Platform; + quarantineRemover: QuarantineRemover; + warn: (message: string) => void; + onProgress?: (downloaded: number, total: number) => void; +} + async function downloadBinary( tag: string, binaryName: string, targetPath: string, - onProgress?: (downloaded: number, total: number) => void, + deps: DownloadBinaryDeps, ): Promise> { + const { platform, quarantineRemover, warn, onProgress } = deps; const url = `https://github.com/${REPO}/releases/download/${tag}/${binaryName}`; let res: Response; @@ -86,12 +130,8 @@ async function downloadBinary( await chmod(tmpPath, 0o755); await rename(tmpPath, targetPath); - if (process.platform === "darwin") { - try { - Bun.spawnSync(["xattr", "-d", "com.apple.quarantine", targetPath]); - } catch { - // ignore — quarantine attribute may not exist - } + if (platform === "darwin") { + tryRemoveMacosQuarantine(targetPath, { remover: quarantineRemover, warn }); } } catch (err) { return R.err(new Error(`Post-download setup failed: ${err instanceof Error ? err.message : String(err)}`)); @@ -133,7 +173,7 @@ export function selfUpdateCommand(container: Container) { spinner.message(`Downloading ${latest.tag}...`); - const binaryResult = detectBinaryName(); + const binaryResult = detectBinaryName(process.platform, process.arch); if (R.isErr(binaryResult)) { spinner.stop(pc.red("Failed")); throw new CommandError(binaryResult.error.message, EXIT_FAILURE); @@ -143,16 +183,21 @@ export function selfUpdateCommand(container: Container) { const execPath = process.execPath; let lastRender = 0; - const downloadResult = await downloadBinary(latest.tag, binaryName, execPath, (downloaded, total) => { - const now = Date.now(); - if (now - lastRender < 200) return; - lastRender = now; - const current = formatMb(downloaded); - const message = - total > 0 - ? `Downloading ${latest.tag}... ${current}/${formatMb(total)} MB` - : `Downloading ${latest.tag}... ${current} MB`; - spinner.message(message); + const downloadResult = await downloadBinary(latest.tag, binaryName, execPath, { + platform: process.platform, + quarantineRemover: defaultQuarantineRemover, + warn: (message) => ui.warn(message), + onProgress: (downloaded, total) => { + const now = Date.now(); + if (now - lastRender < 200) return; + lastRender = now; + const current = formatMb(downloaded); + const message = + total > 0 + ? `Downloading ${latest.tag}... ${current}/${formatMb(total)} MB` + : `Downloading ${latest.tag}... ${current} MB`; + spinner.message(message); + }, }); if (R.isErr(downloadResult)) { spinner.stop(pc.red("Failed")); From ec3b1c583ad6cb0c04fc791e57814fce9c58f032 Mon Sep 17 00:00:00 2001 From: Evgeniy Podivilov Date: Sat, 20 Jun 2026 21:51:18 +0100 Subject: [PATCH 2/2] =?UTF-8?q?chore(self-update):=20address=20review=20?= =?UTF-8?q?=E2=80=94=20guard=20sync=20throws,=20tighten=20tests?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - tryRemoveMacosQuarantine now try/catches the injected remover so a buggy or future remover that throws synchronously still degrades to a warning instead of failing the update, matching the documented best-effort contract. - Add positive test for detectBinaryName("darwin", "x64") — previously only darwin/arm64 and linux/x64 had positive assertions. - Replace misleading "non-Error" test (which actually passed an Error) with two tests that genuinely exercise the synchronous-throw path: one with a thrown Error, one with a thrown non-Error primitive. --- src/cli/commands/self-update.test.ts | 44 +++++++++++++++++++++++----- src/cli/commands/self-update.ts | 10 ++++++- 2 files changed, 46 insertions(+), 8 deletions(-) diff --git a/src/cli/commands/self-update.test.ts b/src/cli/commands/self-update.test.ts index 7dac730..1b55e59 100644 --- a/src/cli/commands/self-update.test.ts +++ b/src/cli/commands/self-update.test.ts @@ -51,6 +51,14 @@ describe("detectBinaryName", () => { } }); + test("darwin/x64 → wt-darwin-x64", () => { + const result = detectBinaryName("darwin", "x64"); + expect(R.isOk(result)).toBe(true); + if (R.isOk(result)) { + expect(result.data).toBe("wt-darwin-x64"); + } + }); + test("linux/x64 → wt-linux-x64", () => { const result = detectBinaryName("linux", "x64"); expect(R.isOk(result)).toBe(true); @@ -89,16 +97,38 @@ describe("tryRemoveMacosQuarantine", () => { expect(warnings[0]).toContain("/path/to/wt"); }); - test("remover errs with non-Error → message still surfaces", () => { + test("remover throws synchronously → warning logged, no throw", () => { const warnings: string[] = []; - const remover: QuarantineRemover = () => R.err(new Error("exited with code 1")); + const remover: QuarantineRemover = () => { + throw new Error("boom from a buggy remover"); + }; - tryRemoveMacosQuarantine("/usr/local/bin/wt", { - remover, - warn: (m) => warnings.push(m), - }); + expect(() => + tryRemoveMacosQuarantine("/usr/local/bin/wt", { + remover, + warn: (m) => warnings.push(m), + }), + ).not.toThrow(); - expect(warnings[0]).toContain("exited with code 1"); + expect(warnings).toHaveLength(1); + expect(warnings[0]).toContain("boom from a buggy remover"); expect(warnings[0]).toContain("/usr/local/bin/wt"); }); + + test("remover throws a non-Error → message still surfaces", () => { + const warnings: string[] = []; + const remover: QuarantineRemover = () => { + throw "raw string failure"; + }; + + expect(() => + tryRemoveMacosQuarantine("/usr/local/bin/wt", { + remover, + warn: (m) => warnings.push(m), + }), + ).not.toThrow(); + + expect(warnings).toHaveLength(1); + expect(warnings[0]).toContain("raw string failure"); + }); }); diff --git a/src/cli/commands/self-update.ts b/src/cli/commands/self-update.ts index b7ea4a8..a398a06 100644 --- a/src/cli/commands/self-update.ts +++ b/src/cli/commands/self-update.ts @@ -55,7 +55,15 @@ export function tryRemoveMacosQuarantine( targetPath: string, deps: { remover: QuarantineRemover; warn: (message: string) => void }, ): void { - const result = deps.remover(targetPath); + let result: Result; + try { + result = deps.remover(targetPath); + } catch (err) { + // Defensive: the contract says the remover returns a Result, but if an + // injected/buggy remover throws synchronously, we must still degrade to + // a warning rather than fail the update. + result = R.err(err instanceof Error ? err : new Error(String(err))); + } if (R.isErr(result)) { deps.warn( `Could not remove macOS quarantine attribute (${result.error.message}); macOS Gatekeeper may block the new binary until you run \`xattr -d com.apple.quarantine ${targetPath}\` manually.`,