From 06cad4384edc5a3237cd62988525fb11a94fecdc Mon Sep 17 00:00:00 2001 From: aMgLn Date: Wed, 2 Sep 2026 17:57:15 +0900 Subject: [PATCH] =?UTF-8?q?=EC=A0=81=EB=8C=80=EC=A0=81=20=EA=B2=80?= =?UTF-8?q?=ED=86=A0=EC=97=90=EC=84=9C=20=EB=B0=9C=EA=B2=AC=EB=90=9C=20?= =?UTF-8?q?=EC=9E=90=EC=B2=B4=20=EC=97=85=EB=8D=B0=EC=9D=B4=ED=8A=B8/?= =?UTF-8?q?=ED=94=8C=EB=9F=AC=EA=B7=B8=EC=9D=B8=20=EC=84=A4=EC=B9=98=20?= =?UTF-8?q?=EC=B7=A8=EC=95=BD=EC=A0=90=20=EB=B0=8F=20=EC=8B=A0=EB=A2=B0?= =?UTF-8?q?=EC=84=B1=20=EB=B2=84=EA=B7=B8=20=EC=88=98=EC=A0=95?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit /goal 현재 구현 상태를 적대적으로 검토해 줘 — code-review 스킬로 wire-paseo/ tag-driven release/멀티 에이전트 감지/자체 업데이트를 검토, 8개 발견 사항을 직접 코드로 재검증한 뒤 전부 수정함: - (심각) 다운로드 파일 경로가 공유 /tmp에 고정된 예측 가능한 파일명이었음 (TOCTOU/심링크 공격 가능 + 동시 실행 시 서로 덮어씀) — bin.ts에 withScratchDir 헬퍼 추가, mkdtemp로 매번 예측 불가능한 디렉터리를 만들어 다운로드하도록 수정 - (심각) selfReplaceBinary의 주석은 "같은 디렉터리 rename이라 원자적"이라고 했지만 실제 호출부는 tmpdir()(다운로드 위치)와 process.execPath(설치 위치)를 그대로 rename해 EXDEV로 조용히 깨질 수 있었음 — trash.server.ts에 이미 있던 renameOrCopy 패턴을 그대로 재사용해 EXDEV 시 copy+unlink로 대체. 윈도우 쪽은 두 번째 rename이 실패하면(백신 잠금 등) 아무 실행 파일도 안 남는 문제가 있어, 실패 시 원래 바이너리를 그대로 롤백하도록 수정 - (심각) extractPluginArchive가 tar 성공 여부 확인 전에 기존 설치를 먼저 삭제해, 손상된 아카이브나 디스크 부족 시 이전에 잘 작동하던 플러그인까지 날아갔음 — 스테이징 디렉터리에 먼저 풀고 tar가 실제로 성공한 뒤에만 destDir을 교체하도록 수정 (실패 시 기존 설치가 그대로 남는 것을 실제 파일시스템 테스트로 확인) - (중간) 다운로드가 200 OK인데 중간에 끊긴 경우를 검증하지 않아 손상된 바이너리/아카이브를 그대로 설치할 수 있었음 — Content-Length 대비 실제 수신 바이트 수를 검증하는 downloadFile 헬퍼를 새로 만들어 두 다운로드 경로(플러그인 아카이브, CLI 바이너리)가 공유하도록 함(기존 중복 코드도 같이 해소) - (중간) compareVersions가 비숫자 버전(사전 릴리스 태그 등)에서 조용히 NaN을 반환해 실제 업데이트가 영원히 안 보이게 될 수 있었음 — InvalidVersionError를 던지도록 하고, isUpdateAvailable은 비교 자체가 안 되면 "업데이트 있음"쪽으로 안전하게 폴백하도록 수정 - (경미) "백그라운드" 체크라고 문서화했지만 실제로는 매 명령 종료 전에 최대 3초까지 동기적으로 기다렸음 — 스크립트/CI에서 예기치 않게 멈출 수 있어 CI 환경변수가 설정된 경우 건너뛰도록 수정(이 테스트 스위트 자체가 GitHub Actions에서 CI=true로 실행되므로, 기존 케이스들이 깨지지 않도록 각 테스트에서 CI 환경변수를 명시적으로 지우고 시작하도록 함) - 죽은 코드였던 pluginArchiveExists 제거 실제로 재검증함: v0.1.0 바이너리 사본에서 실제 최신 릴리스(v0.2.1, 이번 검토 도중 사용자가 PR #4를 머지하고 새로 태그한 것으로 확인됨)로 진짜 자체 업데이트 성공, wire-paseo도 새 스테이징 방식으로 실제 paseo plugin install까지 정상 동작 확인. 테스트 27개 추가(전체 134개 통과). --- packages/cli/src/cli/bin.ts | 67 ++++++++----- packages/cli/src/core/download.server.test.ts | 70 ++++++++++++++ packages/cli/src/core/download.server.ts | 33 +++++++ .../cli/src/core/paseo-wire.server.test.ts | 39 +++++++- packages/cli/src/core/paseo-wire.server.ts | 35 +++---- packages/cli/src/core/update.server.test.ts | 96 ++++++++++++++++++- packages/cli/src/core/update.server.ts | 96 +++++++++++++------ 7 files changed, 359 insertions(+), 77 deletions(-) create mode 100644 packages/cli/src/core/download.server.test.ts create mode 100644 packages/cli/src/core/download.server.ts diff --git a/packages/cli/src/cli/bin.ts b/packages/cli/src/cli/bin.ts index 01bd971..6b292a8 100755 --- a/packages/cli/src/cli/bin.ts +++ b/packages/cli/src/cli/bin.ts @@ -1,4 +1,5 @@ #!/usr/bin/env -S npx tsx +import { mkdtemp, rm } from "node:fs/promises"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { AiderAdapter } from "../core/aider-adapter.server.js"; @@ -82,6 +83,22 @@ function printJson(value: unknown): void { console.log(JSON.stringify(value, null, 2)); } +/** + * Runs `fn` with a fresh, uniquely-named scratch directory, cleaned up afterward either way. Used for + * downloads that get replaced/installed into their real destination right after — a fixed, predictable + * filename directly under the shared OS temp dir would let another local user pre-place a symlink there + * and have the download silently overwrite whatever it points at (no O_EXCL); `mkdtemp`'s random suffix + * means there's nothing for an attacker to predict and pre-create in advance. + */ +async function withScratchDir(fn: (dir: string) => Promise): Promise { + const dir = await mkdtemp(join(tmpdir(), "sessionforge-")); + try { + return await fn(dir); + } finally { + await rm(dir, { recursive: true, force: true }); + } +} + async function cmdDiscover(): Promise { const store = new SessionStore(); try { @@ -279,27 +296,29 @@ async function cmdWirePaseo(args: ParsedArgs): Promise { return; } - const archivePath = join(tmpdir(), PLUGIN_ARCHIVE_NAME); - console.log(`Downloading the v${version} Paseo plugin release asset...`); - await downloadPluginArchive(version, archivePath); + await withScratchDir(async (scratchDir) => { + const archivePath = join(scratchDir, PLUGIN_ARCHIVE_NAME); + console.log(`Downloading the v${version} Paseo plugin release asset...`); + await downloadPluginArchive(version, archivePath); - const installDir = pluginInstallDir(); - console.log(`Extracting to ${installDir}...`); - await extractPluginArchive(archivePath, installDir); + const installDir = pluginInstallDir(); + console.log(`Extracting to ${installDir}...`); + await extractPluginArchive(archivePath, installDir); - const id = flagString(args.flags, "id") ?? DEFAULT_PLUGIN_ID; - console.log("Installing via `paseo plugin install`..."); - const result = await installPluginDirectory(installDir, id); + const id = flagString(args.flags, "id") ?? DEFAULT_PLUGIN_ID; + console.log("Installing via `paseo plugin install`..."); + const result = await installPluginDirectory(installDir, id); - if (result.status !== "running") { - console.error(`\nPlugin installed but is not running (status: ${result.status}).`); - if (result.error) console.error(result.error); - console.error(`Check \`paseo plugin logs ${id}\` for details.`); - process.exitCode = 1; - return; - } + if (result.status !== "running") { + console.error(`\nPlugin installed but is not running (status: ${result.status}).`); + if (result.error) console.error(result.error); + console.error(`Check \`paseo plugin logs ${id}\` for details.`); + process.exitCode = 1; + return; + } - console.log(`\nSessionForge is wired into Paseo (plugin id: ${id}, status: running).`); + console.log(`\nSessionForge is wired into Paseo (plugin id: ${id}, status: running).`); + }); } async function cmdPaseoStatus(): Promise { @@ -360,12 +379,14 @@ async function cmdUpdate(): Promise { } const isWindows = process.platform === "win32"; - const newBinaryPath = join(tmpdir(), `sessionforge-update-${latest}${isWindows ? ".exe" : ""}`); - console.log(`Downloading v${latest}...`); - await downloadCliBinary(latest, newBinaryPath); - - console.log("Installing..."); - await selfReplaceBinary(newBinaryPath, process.execPath); + await withScratchDir(async (scratchDir) => { + const newBinaryPath = join(scratchDir, `sessionforge-update-${latest}${isWindows ? ".exe" : ""}`); + console.log(`Downloading v${latest}...`); + await downloadCliBinary(latest, newBinaryPath); + + console.log("Installing..."); + await selfReplaceBinary(newBinaryPath, process.execPath); + }); console.log(`Updated to v${latest}. Run \`sessionforge wire-paseo\` too if you use the Paseo plugin, to keep it in sync.`); } diff --git a/packages/cli/src/core/download.server.test.ts b/packages/cli/src/core/download.server.test.ts new file mode 100644 index 0000000..ef55fa2 --- /dev/null +++ b/packages/cli/src/core/download.server.test.ts @@ -0,0 +1,70 @@ +import { mkdtemp, readFile, rm } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { downloadFile } from "./download.server.js"; + +describe("downloadFile", () => { + let root: string; + + beforeEach(async () => { + root = await mkdtemp(join(tmpdir(), "sessionforge-download-")); + }); + + afterEach(async () => { + await rm(root, { recursive: true, force: true }); + vi.unstubAllGlobals(); + }); + + it("writes the response body to the destination path", async () => { + vi.stubGlobal( + "fetch", + vi.fn(async () => new Response("hello world", { status: 200 })), + ); + + const destPath = join(root, "file.bin"); + await downloadFile("https://example.com/file.bin", destPath); + + expect(await readFile(destPath, "utf8")).toBe("hello world"); + }); + + it("throws with the status when the request itself fails", async () => { + vi.stubGlobal( + "fetch", + vi.fn(async () => new Response(null, { status: 404, statusText: "Not Found" })), + ); + + await expect(downloadFile("https://example.com/file.bin", join(root, "file.bin"))).rejects.toThrow(/404/); + }); + + it("succeeds silently when the server sends no Content-Length to check against", async () => { + vi.stubGlobal( + "fetch", + vi.fn(async () => new Response("no length header on this one", { status: 200 })), + ); + + await expect(downloadFile("https://example.com/file.bin", join(root, "file.bin"))).resolves.toBeUndefined(); + }); + + it("throws when fewer bytes arrive than the server's own Content-Length promised — a truncated transfer", async () => { + vi.stubGlobal( + "fetch", + vi.fn(async () => new Response("short", { status: 200, headers: { "content-length": "9999" } })), + ); + + await expect(downloadFile("https://example.com/file.bin", join(root, "file.bin"))).rejects.toThrow(/incomplete/i); + }); + + it("succeeds when the transferred bytes match a real Content-Length", async () => { + const body = "exact content"; + vi.stubGlobal( + "fetch", + vi.fn(async () => new Response(body, { status: 200, headers: { "content-length": String(Buffer.byteLength(body)) } })), + ); + + const destPath = join(root, "file.bin"); + await downloadFile("https://example.com/file.bin", destPath); + + expect(await readFile(destPath, "utf8")).toBe(body); + }); +}); diff --git a/packages/cli/src/core/download.server.ts b/packages/cli/src/core/download.server.ts new file mode 100644 index 0000000..9b41df4 --- /dev/null +++ b/packages/cli/src/core/download.server.ts @@ -0,0 +1,33 @@ +import { createWriteStream } from "node:fs"; +import { pipeline } from "node:stream/promises"; +import { Readable, Transform } from "node:stream"; + +/** + * Fetches a URL and streams it to disk, verifying the transferred byte count against the response's own + * Content-Length (when the server sends one) before treating the write as successful — a connection reset + * or truncated proxy response can otherwise leave a corrupt, incomplete file on disk that still looks like + * a normal successful download to a caller that only checked `response.ok`. Shared by + * `paseo-wire.server.ts` (plugin archive) and `update.server.ts` (CLI binary) since both hand their result + * straight to something high-stakes: `paseo plugin install` or replacing the running executable. + */ +export async function downloadFile(url: string, destPath: string): Promise { + const response = await fetch(url); + if (!response.ok || !response.body) { + throw new Error(`Download failed (${response.status} ${response.statusText}): ${url}`); + } + + const expectedLength = response.headers.get("content-length"); + let receivedBytes = 0; + const countBytes = new Transform({ + transform(chunk: Buffer, _encoding, callback) { + receivedBytes += chunk.length; + callback(null, chunk); + }, + }); + + await pipeline(Readable.fromWeb(response.body as import("node:stream/web").ReadableStream), countBytes, createWriteStream(destPath)); + + if (expectedLength !== null && receivedBytes !== Number(expectedLength)) { + throw new Error(`Download incomplete: expected ${expectedLength} bytes, got ${receivedBytes} (${url})`); + } +} diff --git a/packages/cli/src/core/paseo-wire.server.test.ts b/packages/cli/src/core/paseo-wire.server.test.ts index 29b2388..d532f44 100644 --- a/packages/cli/src/core/paseo-wire.server.test.ts +++ b/packages/cli/src/core/paseo-wire.server.test.ts @@ -1,4 +1,5 @@ -import { mkdtemp, rm, writeFile } from "node:fs/promises"; +import { existsSync, writeFileSync } from "node:fs"; +import { mkdir, mkdtemp, readFile, rm, writeFile } from "node:fs/promises"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; @@ -94,7 +95,6 @@ describe("paseo-wire", () => { await downloadPluginArchive("1.2.3", destPath); expect(fetchMock).toHaveBeenCalledOnce(); - const { readFile } = await import("node:fs/promises"); expect(await readFile(destPath, "utf8")).toBe("fake tarball contents"); }); @@ -109,7 +109,7 @@ describe("paseo-wire", () => { }); describe("extractPluginArchive", () => { - it("wipes the destination and shells out to tar", async () => { + it("extracts into a staging directory alongside the destination, not straight into it", async () => { const destDir = join(root, "dest"); let sawTarArgs: string[] = []; execFileHandler = (cmd, args, callback) => { @@ -120,7 +120,37 @@ describe("paseo-wire", () => { await extractPluginArchive(join(root, "archive.tar.gz"), destDir); - expect(sawTarArgs).toEqual(["-xzf", join(root, "archive.tar.gz"), "-C", destDir]); + expect(sawTarArgs).toEqual(["-xzf", join(root, "archive.tar.gz"), "-C", `${destDir}.staging`]); + }); + + it("leaves a previous working install untouched when tar fails, instead of wiping it first", async () => { + const destDir = join(root, "dest"); + await mkdir(destDir, { recursive: true }); + await writeFile(join(destDir, "still-here.txt"), "previous working install"); + + execFileHandler = (_cmd, _args, callback) => callback(new Error("tar: unexpected end of file")); + + await expect(extractPluginArchive(join(root, "archive.tar.gz"), destDir)).rejects.toThrow(); + + expect(await readFile(join(destDir, "still-here.txt"), "utf8")).toBe("previous working install"); + }); + + it("replaces the destination with the newly-extracted content once tar actually succeeds", async () => { + const destDir = join(root, "dest"); + await mkdir(destDir, { recursive: true }); + await writeFile(join(destDir, "old.txt"), "old content"); + + execFileHandler = (_cmd, args, callback) => { + // Simulate tar really writing into the staging directory it was told to extract into. + const stagingDir = args[args.indexOf("-C") + 1]; + writeFileSync(join(stagingDir, "new.txt"), "new content"); + callback(null, { stdout: "", stderr: "" }); + }; + + await extractPluginArchive(join(root, "archive.tar.gz"), destDir); + + expect(existsSync(join(destDir, "old.txt"))).toBe(false); + expect(await readFile(join(destDir, "new.txt"), "utf8")).toBe("new content"); }); }); @@ -168,7 +198,6 @@ describe("paseo-wire", () => { describe("getInstalledPluginVersion", () => { it("reads the version marker scripts/package-plugin.mjs bakes into the packaged bundle", async () => { - const { writeFile } = await import("node:fs/promises"); await writeFile(join(root, ".sessionforge-version"), "0.3.0\n"); expect(await getInstalledPluginVersion(root)).toBe("0.3.0"); diff --git a/packages/cli/src/core/paseo-wire.server.ts b/packages/cli/src/core/paseo-wire.server.ts index e802e25..94c7287 100644 --- a/packages/cli/src/core/paseo-wire.server.ts +++ b/packages/cli/src/core/paseo-wire.server.ts @@ -1,11 +1,9 @@ import { execFile } from "node:child_process"; -import { createWriteStream, existsSync } from "node:fs"; -import { mkdir, readFile, rm } from "node:fs/promises"; +import { mkdir, readFile, rename, rm } from "node:fs/promises"; import { homedir } from "node:os"; import { join } from "node:path"; -import { pipeline } from "node:stream/promises"; -import { Readable } from "node:stream"; import { promisify } from "node:util"; +import { downloadFile } from "./download.server.js"; const execFileAsync = promisify(execFile); @@ -57,19 +55,26 @@ export async function arePluginsEnabled(): Promise { * always wires up the plugin build it actually shipped with, not a possibly-incompatible newer one. */ export async function downloadPluginArchive(version: string, destPath: string): Promise { const url = `https://github.com/4mGLn/sessionforge/releases/download/v${version}/${PLUGIN_ARCHIVE_NAME}`; - const response = await fetch(url); - if (!response.ok || !response.body) { - throw new Error(`Download failed (${response.status} ${response.statusText}): ${url}`); - } - await pipeline(Readable.fromWeb(response.body as import("node:stream/web").ReadableStream), createWriteStream(destPath)); + await downloadFile(url, destPath); } -/** tar ships on Linux/macOS by default and as bsdtar on Windows 10 1803+ / Windows 11 — the same - * assumption install.sh/install.ps1 and this project's other platform-support claims already make. */ +/** + * tar ships on Linux/macOS by default and as bsdtar on Windows 10 1803+ / Windows 11 — the same assumption + * install.sh/install.ps1 and this project's other platform-support claims already make. + * + * Extracts to a staging directory first and only replaces `destDir` after `tar` has actually succeeded — + * a corrupt or truncated archive (bad download, disk full mid-extract) then just fails cleanly, instead of + * first wiping out a previously-working plugin install and leaving Paseo pointed at a broken directory. + */ export async function extractPluginArchive(archivePath: string, destDir: string): Promise { + const stagingDir = `${destDir}.staging`; + await rm(stagingDir, { recursive: true, force: true }); + await mkdir(stagingDir, { recursive: true }); + await execFileAsync("tar", ["-xzf", archivePath, "-C", stagingDir]); + + // Only reached once tar has proven the archive is valid. await rm(destDir, { recursive: true, force: true }); - await mkdir(destDir, { recursive: true }); - await execFileAsync("tar", ["-xzf", archivePath, "-C", destDir]); + await rename(stagingDir, destDir); } interface PluginInstallResult { @@ -91,10 +96,6 @@ export async function getPluginStatus(id: string = DEFAULT_PLUGIN_ID): Promise

plugin.id === id) ?? null; } -export function pluginArchiveExists(path: string): boolean { - return existsSync(path); -} - const PLUGIN_VERSION_FILE = ".sessionforge-version"; /** diff --git a/packages/cli/src/core/update.server.test.ts b/packages/cli/src/core/update.server.test.ts index ffb37cb..5b7b90b 100644 --- a/packages/cli/src/core/update.server.test.ts +++ b/packages/cli/src/core/update.server.test.ts @@ -1,17 +1,36 @@ -import { statSync, writeFileSync } from "node:fs"; -import { mkdtemp as mkdtempAsync, readFile, rm } from "node:fs/promises"; +import { existsSync, statSync, writeFileSync } from "node:fs"; +import { mkdtemp as mkdtempAsync, readFile, rename as realRename, rm } from "node:fs/promises"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; -import { + +// Lets specific tests simulate a cross-filesystem rename (EXDEV) without needing two real filesystems — +// falls through to the real implementation for every test that doesn't set an override. +let renameOverride: ((source: string, dest: string) => Promise) | null = null; +vi.mock("node:fs/promises", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + rename: (source: string, dest: string) => (renameOverride ? renameOverride(source, dest) : actual.rename(source, dest)), + }; +}); + +const { checkForUpdateCached, compareVersions, downloadCliBinary, getLatestReleaseVersion, + InvalidVersionError, isUpdateAvailable, selfReplaceBinary, targetTriple, -} from "./update.server.js"; +} = await import("./update.server.js"); + +function exdevError(): NodeJS.ErrnoException { + const error = new Error("EXDEV: cross-device link not permitted") as NodeJS.ErrnoException; + error.code = "EXDEV"; + return error; +} function setPlatform(platform: NodeJS.Platform): void { Object.defineProperty(process, "platform", { value: platform, configurable: true }); @@ -28,6 +47,12 @@ describe("compareVersions", () => { expect(compareVersions("1.2", "1.2.0")).toBe(0); expect(compareVersions("1.3", "1.2.9")).toBeGreaterThan(0); }); + + it("throws InvalidVersionError instead of silently comparing as NaN for a non-numeric component", () => { + // A plain numeric split+compare would make "0.3.0-rc" parse to [0, 3, NaN] — NaN is never > 0, so a + // real update would go permanently invisible to isUpdateAvailable without this ever raising an error. + expect(() => compareVersions("0.3.0-rc", "0.2.0")).toThrow(InvalidVersionError); + }); }); describe("isUpdateAvailable", () => { @@ -46,6 +71,10 @@ describe("isUpdateAvailable", () => { it("is always true for a dev-main build — any real release counts as newer", () => { expect(isUpdateAvailable("dev-main", "0.0.1")).toBe(true); }); + + it("fails toward 'yes, update available' rather than silently hiding an unparseable version", () => { + expect(isUpdateAvailable("0.2.0", "0.3.0-rc.1")).toBe(true); + }); }); describe("targetTriple", () => { @@ -195,21 +224,69 @@ describe("selfReplaceBinary", () => { expect(await readFile(current, "utf8")).toBe("new"); }); + + afterEach(() => { + renameOverride = null; + }); + + it("falls back to copy+unlink on EXDEV — e.g. the download and install location are on different filesystems", async () => { + setPlatform("linux"); + const current = join(root, "sessionforge"); + const incoming = join(root, "sessionforge-new"); + writeFileSync(current, "old"); + writeFileSync(incoming, "new"); + + renameOverride = async () => { + throw exdevError(); + }; + + await selfReplaceBinary(incoming, current); + + expect(await readFile(current, "utf8")).toBe("new"); + expect(existsSync(incoming)).toBe(false); // source removed after the copy, same as a real rename would leave it + }); + + it("rolls the original binary back into place on windows if putting the new one there fails, instead of leaving nothing runnable", async () => { + setPlatform("win32"); + const current = join(root, "sessionforge.exe"); + const incoming = join(root, "sessionforge-new.exe"); + writeFileSync(current, "old"); + writeFileSync(incoming, "new"); + + let call = 0; + renameOverride = async (source, dest) => { + call += 1; + if (call === 2) throw new Error("simulated: antivirus lock"); + return realRename(source, dest); + }; + + await expect(selfReplaceBinary(incoming, current)).rejects.toThrow(/antivirus lock/); + + // A failed update must be a no-op, never a bricked install — the original binary has to still be + // runnable at its original path. + expect(await readFile(current, "utf8")).toBe("old"); + }); }); describe("checkForUpdateCached", () => { let root: string; let previousHome: string | undefined; let previousUserProfile: string | undefined; + let previousCi: string | undefined; beforeEach(async () => { root = await mkdtempAsync(join(tmpdir(), "sessionforge-update-cache-")); previousHome = process.env.HOME; previousUserProfile = process.env.USERPROFILE; + previousCi = process.env.CI; // node:os's homedir() reads USERPROFILE (not HOME) on Windows — setting both keeps the cache file // sandboxed to `root` regardless of which real OS runs this test. process.env.HOME = root; process.env.USERPROFILE = root; + // This suite itself normally runs inside GitHub Actions, which sets CI=true by default — unset it here + // so the non-CI-skip tests below actually exercise real behavior instead of hitting the new CI guard + // regardless of what they're testing. + delete process.env.CI; }); afterEach(async () => { @@ -217,6 +294,8 @@ describe("checkForUpdateCached", () => { else process.env.HOME = previousHome; if (previousUserProfile === undefined) delete process.env.USERPROFILE; else process.env.USERPROFILE = previousUserProfile; + if (previousCi === undefined) delete process.env.CI; + else process.env.CI = previousCi; await rm(root, { recursive: true, force: true }); vi.unstubAllGlobals(); }); @@ -229,6 +308,15 @@ describe("checkForUpdateCached", () => { expect(fetchMock).not.toHaveBeenCalled(); }); + it("returns null immediately in CI, without any network call — no one reads the notice in a pipeline", async () => { + process.env.CI = "true"; + const fetchMock = vi.fn(); + vi.stubGlobal("fetch", fetchMock); + + expect(await checkForUpdateCached("0.3.0")).toBeNull(); + expect(fetchMock).not.toHaveBeenCalled(); + }); + it("performs a real check and reports an available update", async () => { vi.stubGlobal( "fetch", diff --git a/packages/cli/src/core/update.server.ts b/packages/cli/src/core/update.server.ts index 83dd17b..ee6ef40 100644 --- a/packages/cli/src/core/update.server.ts +++ b/packages/cli/src/core/update.server.ts @@ -1,9 +1,8 @@ -import { chmodSync, createWriteStream } from "node:fs"; -import { mkdir, readFile, rename, rm, writeFile } from "node:fs/promises"; +import { chmodSync } from "node:fs"; +import { copyFile, mkdir, readFile, rename, rm, unlink, writeFile } from "node:fs/promises"; import { arch, homedir, platform } from "node:os"; import { join } from "node:path"; -import { pipeline } from "node:stream/promises"; -import { Readable } from "node:stream"; +import { downloadFile } from "./download.server.js"; const REPO = "4mGLn/sessionforge"; const UPDATE_CHECK_CACHE_TTL_MS = 24 * 60 * 60 * 1000; // 24 hours — one real network check per day at most @@ -27,11 +26,24 @@ export function targetTriple(): string { throw new Error(`Unsupported platform/arch combination for self-update: ${p}/${a}`); } +/** Thrown by compareVersions for anything that isn't a plain dotted-numeric version (e.g. a pre-release + * tag like "0.3.0-rc.1") — better than silently comparing as NaN, which is neither > 0 nor < 0 nor === 0 + * and would make a real update invisible to isUpdateAvailable without ever raising an error anywhere. */ +export class InvalidVersionError extends Error {} + +function parseVersionParts(version: string): number[] { + const parts = version.split(".").map(Number); + if (parts.some((part) => Number.isNaN(part))) { + throw new InvalidVersionError(`Not a plain dotted-numeric version: "${version}"`); + } + return parts; +} + /** Compares two dotted-numeric version strings (e.g. "0.10.2" vs "0.2.9") field by field, not * lexicographically — a plain string compare would wrongly rank "0.10.0" below "0.2.0". */ export function compareVersions(a: string, b: string): number { - const partsA = a.split(".").map(Number); - const partsB = b.split(".").map(Number); + const partsA = parseVersionParts(a); + const partsB = parseVersionParts(b); const length = Math.max(partsA.length, partsB.length); for (let i = 0; i < length; i += 1) { const diff = (partsA[i] ?? 0) - (partsB[i] ?? 0); @@ -40,10 +52,19 @@ export function compareVersions(a: string, b: string): number { return 0; } -/** "dev-main" (a local/non-release build) never numerically compares — any real release counts as newer. */ +/** + * "dev-main" (a local/non-release build) never numerically compares — any real release counts as newer. + * A version that fails to parse (see InvalidVersionError) fails toward "yes, an update might be available" + * rather than silently claiming everything's fine when the comparison itself couldn't actually be done. + */ export function isUpdateAvailable(current: string, latest: string): boolean { if (current === "dev-main") return true; - return compareVersions(latest, current) > 0; + try { + return compareVersions(latest, current) > 0; + } catch (error) { + if (error instanceof InvalidVersionError) return true; + throw error; + } } interface GitHubRelease { @@ -73,13 +94,16 @@ interface UpdateCheckCache { } /** - * Rate-limited (24h) background-style check — never throws, so it's safe to call unconditionally at the - * end of any command without risking that command's own output/exit code. Returns null on any failure - * (network down, GitHub unreachable, cache unreadable) or when running a dev-main build, so callers can - * just skip printing anything rather than needing their own error handling. + * Rate-limited (24h) update check appended to the end of most commands. Not a detached background task — + * it genuinely delays process exit by up to BACKGROUND_CHECK_TIMEOUT_MS on a cache-miss day, since the + * whole point is printing its notice before the command's own output is done. Never throws, so it's safe + * to call unconditionally without risking the calling command's own output/exit code. Returns null on any + * failure (network down, GitHub unreachable, cache unreadable), when running a dev-main build, or in CI + * (`CI` env var — a stalled network check has no one to read its notice and just wastes time in a + * pipeline), so callers can just skip printing anything rather than needing their own error handling. */ export async function checkForUpdateCached(currentVersion: string): Promise<{ latestVersion: string; updateAvailable: boolean } | null> { - if (currentVersion === "dev-main") return null; + if (currentVersion === "dev-main" || process.env.CI) return null; try { const cachePath = updateCheckCachePath(); @@ -111,36 +135,52 @@ export async function downloadCliBinary(version: string, destPath: string): Prom const isWindows = platform() === "win32"; const assetName = `sessionforge-${targetTriple()}${isWindows ? ".exe" : ""}`; const url = `https://github.com/${REPO}/releases/download/v${version}/${assetName}`; - const response = await fetch(url); - if (!response.ok || !response.body) { - throw new Error(`Download failed (${response.status} ${response.statusText}): ${url}`); - } - await pipeline(Readable.fromWeb(response.body as import("node:stream/web").ReadableStream), createWriteStream(destPath)); + await downloadFile(url, destPath); if (!isWindows) chmodSync(destPath, 0o755); } +/** rename() across filesystems throws EXDEV — falls back to copy+unlink, since the downloaded file + * (usually under the OS temp dir) and the install location aren't guaranteed to be on the same filesystem + * (e.g. tmpfs /tmp vs a separately-mounted /usr/local/bin). Same pattern trash.server.ts's own + * renameOrCopy already uses for the identical reason. */ +async function renameOrCopy(sourcePath: string, destPath: string): Promise { + try { + await rename(sourcePath, destPath); + } catch (error) { + if ((error as NodeJS.ErrnoException).code !== "EXDEV") throw error; + await copyFile(sourcePath, destPath); + await unlink(sourcePath); + } +} + /** * Replaces the currently-running binary in place with a freshly-downloaded one — the standard * self-updating-CLI pattern (same one rustup/many Go tools use), because a running executable can't just * be overwritten directly on every OS: - * - Linux/macOS: renaming a file that's currently executing is fine — the OS keeps the old inode alive for - * the still-running process, and the path just starts pointing at the new file. A same-directory rename - * (not a cross-filesystem copy) keeps this atomic. + * - Linux/macOS: renaming (or, cross-filesystem, copying) over a file that's currently executing is fine — + * the OS keeps the old inode alive for the still-running process, and the path just starts pointing at + * the new file. * - Windows: a running .exe can't be deleted or overwritten, but it CAN be renamed. So the running binary - * is renamed to a `.old.exe` sibling first, then the downloaded file takes its original name. The - * `.old.exe` is best-effort deleted immediately after (already-renamed-away, so this rarely fails, but a - * file still flushing to disk on a slow machine could keep it locked a moment longer) — a leftover - * `.old.exe` is harmless clutter, not a correctness problem, so a failed cleanup isn't treated as fatal. + * is renamed to a `.old.exe` sibling first, then the downloaded file takes its original name. If that + * second step fails (cross-filesystem copy error, antivirus lock, disk full) the `.old.exe` is renamed + * straight back to the original name so the user isn't left with no runnable binary at all — a failed + * update should be a no-op, never a bricked install. The `.old.exe` is best-effort deleted once the swap + * actually succeeds; a leftover on a rare cleanup failure is harmless clutter, not a correctness problem. */ export async function selfReplaceBinary(newBinaryPath: string, currentExecPath: string): Promise { if (platform() === "win32") { const oldPath = `${currentExecPath}.old.exe`; await rm(oldPath, { force: true }); - await rename(currentExecPath, oldPath); - await rename(newBinaryPath, currentExecPath); + await renameOrCopy(currentExecPath, oldPath); + try { + await renameOrCopy(newBinaryPath, currentExecPath); + } catch (error) { + await renameOrCopy(oldPath, currentExecPath).catch(() => {}); + throw error; + } await rm(oldPath, { force: true }).catch(() => {}); return; } - await rename(newBinaryPath, currentExecPath); + await renameOrCopy(newBinaryPath, currentExecPath); }