From 399cf0cf412e67be051a3046cf1b3842657a0e90 Mon Sep 17 00:00:00 2001 From: saopig1 <4x7sw862st@gmail.com> Date: Fri, 3 Jul 2026 01:45:48 +0800 Subject: [PATCH] fix(spotify): PATH-aware binary presence detection (bin/ or PATH) [whole-branch I3,m1] MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit rustPresent/goPresent and getBackendInfo used existsSync(findX()), which for a bare PATH command name resolves against cwd, not $PATH — so a scoop/choco/cargo/ apt install was invisible and Spotify was gated off. Add a sync PATH-aware resolveExecutable() + isLibrespotPresent()/isGoLibrespotPresent() in binary.ts and route controller.ts and web/server.ts through them. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/music/spotify/binary.test.ts | 127 +++++++++++++++++++++++++++ src/music/spotify/binary.ts | 54 +++++++++++- src/music/spotify/controller.test.ts | 30 ++++--- src/music/spotify/controller.ts | 13 ++- src/web/server.ts | 15 ++-- 5 files changed, 211 insertions(+), 28 deletions(-) diff --git a/src/music/spotify/binary.test.ts b/src/music/spotify/binary.test.ts index a19f51f..640f235 100644 --- a/src/music/spotify/binary.test.ts +++ b/src/music/spotify/binary.test.ts @@ -1,5 +1,7 @@ import { describe, it, expect, vi, afterEach } from "vitest"; import { join } from "node:path"; +import { mkdtempSync, writeFileSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; import { isGoLibrespotSupported, pickGoLibrespotPath, @@ -13,6 +15,9 @@ import { checkLibrespotAvailable, resetLibrespotBinaryCache, __setLibrespotVersionProbe, + resolveExecutable, + isLibrespotPresent, + isGoLibrespotPresent, } from "./binary.js"; const origPlatform = process.platform; @@ -20,12 +25,33 @@ function setPlatform(p: NodeJS.Platform): void { Object.defineProperty(process, "platform", { value: p, configurable: true }); } +// Snapshot the PATH env so PATH-resolution tests can point it at temp dirs and +// restore afterwards. PATHEXT is optional (undefined on posix); track presence. +const origPath = process.env.PATH; +const origPathExt = process.env.PATHEXT; +const tmpDirs: string[] = []; +function mkTmpDir(prefix: string): string { + const dir = mkdtempSync(join(tmpdir(), prefix)); + tmpDirs.push(dir); + return dir; +} + afterEach(() => { setPlatform(origPlatform); __setGoLibrespotVersionProbe(null); resetGoLibrespotBinaryCache(); __setLibrespotVersionProbe(null); resetLibrespotBinaryCache(); + process.env.PATH = origPath; + if (origPathExt === undefined) delete process.env.PATHEXT; + else process.env.PATHEXT = origPathExt; + for (const dir of tmpDirs.splice(0)) { + try { + rmSync(dir, { recursive: true, force: true }); + } catch { + /* ignore */ + } + } }); describe("isGoLibrespotSupported", () => { @@ -234,3 +260,104 @@ describe("checkLibrespotAvailable", () => { expect(probe).toHaveBeenCalledTimes(1); }); }); + +describe("resolveExecutable", () => { + it("returns a bin/-style path (contains a separator) that exists, unchanged", () => { + const dir = mkTmpDir("tsmb-resolve-"); + const full = join(dir, "some-binary"); + writeFileSync(full, "#!/bin/sh\n"); + // The path contains a separator so it is checked directly, not via PATH. + expect(resolveExecutable(full)).toBe(full); + }); + + it("returns null for a bin/-style path that does not exist", () => { + const full = join(tmpdir(), "tsmb-resolve-missing", "nope-binary"); + expect(resolveExecutable(full)).toBeNull(); + }); + + it("resolves a BARE command name found in a PATH directory (the I3 fix)", () => { + // This is the regression the fix targets: a PATH-installed binary (bare + // name, no separator) must resolve against $PATH, not process.cwd(). + const dir = mkTmpDir("tsmb-resolve-path-"); + const exeName = process.platform === "win32" ? "faketool.exe" : "faketool"; + const full = join(dir, exeName); + writeFileSync(full, "#!/bin/sh\n"); + process.env.PATH = dir; + expect(resolveExecutable(exeName)).toBe(full); + }); + + it("returns null for a bare name not present in any PATH directory", () => { + process.env.PATH = mkTmpDir("tsmb-resolve-empty-"); + expect(resolveExecutable("definitely-not-a-real-binary-xyz")).toBeNull(); + }); + + it("appends PATHEXT extensions on win32 to resolve a bare (extensionless) name", () => { + setPlatform("win32"); + const dir = mkTmpDir("tsmb-resolve-pathext-"); + const full = join(dir, "mytool.CMD"); + writeFileSync(full, "@echo hi\n"); + process.env.PATH = dir; + process.env.PATHEXT = ".EXE;.CMD;.BAT;.COM"; + // Bare "mytool" has no extension; win32 resolution must try PATHEXT and + // find mytool.CMD. + expect(resolveExecutable("mytool")).toBe(full); + }); + + it("does not touch PATH for a bin/-style relative path with a separator", () => { + // A candidate with a separator is checked directly even if PATH is empty. + process.env.PATH = ""; + const dir = mkTmpDir("tsmb-resolve-direct-"); + const full = join(dir, "direct-binary"); + writeFileSync(full, "#!/bin/sh\n"); + expect(resolveExecutable(full)).toBe(full); + }); +}); + +describe("isLibrespotPresent (PATH-aware, sync)", () => { + it("true when the bare librespot name resolves on PATH (posix)", () => { + setPlatform("linux"); + const dir = mkTmpDir("tsmb-librespot-path-"); + writeFileSync(join(dir, "librespot"), "#!/bin/sh\n"); + process.env.PATH = dir; + expect(isLibrespotPresent()).toBe(true); + }); + + it("true when librespot.exe resolves on PATH (win32)", () => { + setPlatform("win32"); + const dir = mkTmpDir("tsmb-librespot-win-"); + writeFileSync(join(dir, "librespot.exe"), "MZ\n"); + process.env.PATH = dir; + process.env.PATHEXT = ".EXE;.CMD;.BAT;.COM"; + expect(isLibrespotPresent()).toBe(true); + }); + + it("false when librespot is not on PATH", () => { + setPlatform("linux"); + process.env.PATH = mkTmpDir("tsmb-librespot-empty-"); + expect(isLibrespotPresent()).toBe(false); + }); +}); + +describe("isGoLibrespotPresent (PATH-aware, sync)", () => { + it("true when go-librespot resolves on PATH (linux)", () => { + setPlatform("linux"); + const dir = mkTmpDir("tsmb-go-path-"); + writeFileSync(join(dir, "go-librespot"), "#!/bin/sh\n"); + process.env.PATH = dir; + expect(isGoLibrespotPresent()).toBe(true); + }); + + it("false on non-linux platforms regardless of PATH (unsupported gate)", () => { + setPlatform("win32"); + const dir = mkTmpDir("tsmb-go-win-"); + writeFileSync(join(dir, "go-librespot"), "#!/bin/sh\n"); + process.env.PATH = dir; + expect(isGoLibrespotPresent()).toBe(false); + }); + + it("false on linux when go-librespot is not on PATH", () => { + setPlatform("linux"); + process.env.PATH = mkTmpDir("tsmb-go-empty-"); + expect(isGoLibrespotPresent()).toBe(false); + }); +}); diff --git a/src/music/spotify/binary.ts b/src/music/spotify/binary.ts index c33c409..84476f2 100644 --- a/src/music/spotify/binary.ts +++ b/src/music/spotify/binary.ts @@ -2,12 +2,44 @@ import { execFile } from "node:child_process"; import { promisify } from "node:util"; import { existsSync } from "node:fs"; import { fileURLToPath } from "node:url"; -import { dirname, join } from "node:path"; +import { dirname, join, delimiter } from "node:path"; const execFileAsync = promisify(execFile); const __dirname = dirname(fileURLToPath(import.meta.url)); +/** + * Resolve a command to an existing absolute path, SYNCHRONOUSLY. A candidate + * that already contains a path separator (a project bin/ path) is checked + * directly; a BARE command name (e.g. "librespot" / "go-librespot") is searched + * across the $PATH directories — with PATHEXT extensions appended on win32 — so + * a scoop/choco/cargo/apt PATH install resolves. Returns the resolved path, or + * null when nothing exists. + * + * This is the sync counterpart to checkLibrespotAvailable()/…GoLibrespot… and + * is what the hot-path presence gates (chooseBackend/isAvailable) rely on: a + * bare name must NOT be handed to existsSync() directly, since existsSync + * resolves it against process.cwd() rather than PATH (Bug I3/m1). + */ +export function resolveExecutable(cmd: string): string | null { + if (cmd.includes("/") || cmd.includes("\\")) { + return existsSync(cmd) ? cmd : null; + } + const dirs = (process.env.PATH || "").split(delimiter).filter(Boolean); + const exts = + process.platform === "win32" + ? (process.env.PATHEXT || ".EXE;.CMD;.BAT;.COM").split(";").filter(Boolean) + : [""]; + for (const dir of dirs) { + for (const ext of exts) { + const hasExt = ext && cmd.toLowerCase().endsWith(ext.toLowerCase()); + const full = join(dir, hasExt ? cmd : cmd + ext); + if (existsSync(full)) return full; + } + } + return null; +} + /** * True only on Linux. go-librespot ships Linux-only release binaries and the * sidecar relies on a POSIX FIFO (mkfifo), so the Spotify audio backend is @@ -44,6 +76,16 @@ export function findGoLibrespot(): string { return pickGoLibrespotPath([binPath, "go-librespot"], existsSync); } +/** + * SYNCHRONOUS presence gate for go-librespot: supported platform (linux) AND + * the resolved binary (project bin/ OR a PATH install) actually exists. This is + * what chooseBackend()/isAvailable() must use — NOT existsSync(findGoLibrespot()) + * which cannot see a bare PATH name (Bug I3/m1). + */ +export function isGoLibrespotPresent(): boolean { + return isGoLibrespotSupported() && resolveExecutable(findGoLibrespot()) !== null; +} + // Injectable `--version` probe. Defaults to the real execFile call; tests // override it so checkGoLibrespotAvailable() needs no real binary. Keeps the // public checkGoLibrespotAvailable() signature param-free per the contract. @@ -143,6 +185,16 @@ export function findLibrespot(): string { return pickLibrespotPath([binExe, binBare, exe], existsSync); } +/** + * SYNCHRONOUS presence gate for Rust librespot: supported everywhere AND the + * resolved binary (project bin/ OR a scoop/choco/cargo PATH install) actually + * exists. This is what chooseBackend()/isAvailable() must use — NOT + * existsSync(findLibrespot()) which cannot see a bare PATH name (Bug I3/m1). + */ +export function isLibrespotPresent(): boolean { + return isRustLibrespotSupported() && resolveExecutable(findLibrespot()) !== null; +} + // Injectable `--version` probe. Defaults to the real execFile call; tests // override it so checkLibrespotAvailable() needs no real binary. Keeps the // public checkLibrespotAvailable() signature param-free per the contract. diff --git a/src/music/spotify/controller.test.ts b/src/music/spotify/controller.test.ts index 797ff7a..819a227 100644 --- a/src/music/spotify/controller.test.ts +++ b/src/music/spotify/controller.test.ts @@ -22,16 +22,26 @@ const bin = vi.hoisted(() => ({ rustSupported: true, rustPath: "", })); -vi.mock("./binary.js", () => ({ - isGoLibrespotSupported: () => bin.supported, - findGoLibrespot: () => bin.path, - resetGoLibrespotBinaryCache: () => {}, - checkGoLibrespotAvailable: async () => bin.supported && !!bin.path, - isRustLibrespotSupported: () => bin.rustSupported, - findLibrespot: () => bin.rustPath, - resetLibrespotBinaryCache: () => {}, - checkLibrespotAvailable: async () => bin.rustSupported && !!bin.rustPath, -})); +vi.mock("./binary.js", async () => { + // existsSync mirrors the real resolveExecutable(find*()) result for the + // bin/-style temp paths this suite uses (existingBin exists, missingBin does + // not), keeping the chooseBackend/isAvailable matrix behavior-identical. + const { existsSync } = await import("node:fs"); + return { + isGoLibrespotSupported: () => bin.supported, + findGoLibrespot: () => bin.path, + resetGoLibrespotBinaryCache: () => {}, + checkGoLibrespotAvailable: async () => bin.supported && !!bin.path, + isRustLibrespotSupported: () => bin.rustSupported, + findLibrespot: () => bin.rustPath, + resetLibrespotBinaryCache: () => {}, + checkLibrespotAvailable: async () => bin.rustSupported && !!bin.rustPath, + // PATH-aware presence gates (Bug I3): supported gate AND the resolved + // binary actually exists on disk. + isGoLibrespotPresent: () => bin.supported && existsSync(bin.path), + isLibrespotPresent: () => bin.rustSupported && existsSync(bin.rustPath), + }; +}); // Capture options the DEFAULT factory hands to the real GoLibrespotBackend so // we can assert per-bot ports (Fix 3) are threaded through. Every other test diff --git a/src/music/spotify/controller.ts b/src/music/spotify/controller.ts index 2fa2e0d..3c41d2a 100644 --- a/src/music/spotify/controller.ts +++ b/src/music/spotify/controller.ts @@ -15,12 +15,7 @@ import type { SpotifyTrackEndedEvent, SpotifyNowPlaying, } from "./backend.js"; -import { - isGoLibrespotSupported, - findGoLibrespot, - isRustLibrespotSupported, - findLibrespot, -} from "./binary.js"; +import { isGoLibrespotPresent, isLibrespotPresent } from "./binary.js"; import { GoLibrespotBackend } from "./go-librespot.js"; import { RustLibrespotBackend } from "./rust-librespot.js"; import { @@ -151,11 +146,13 @@ export class SpotifyController extends EventEmitter { } private goPresent(): boolean { - return isGoLibrespotSupported() && existsSync(findGoLibrespot()); + // PATH-aware, sync presence gate (Bug I3): a bare PATH-installed binary is + // resolved against $PATH, not existsSync()'d against process.cwd(). + return isGoLibrespotPresent(); } private rustPresent(): boolean { - return isRustLibrespotSupported() && existsSync(findLibrespot()); + return isLibrespotPresent(); } /** diff --git a/src/web/server.ts b/src/web/server.ts index 720bbc1..42c9f66 100755 --- a/src/web/server.ts +++ b/src/web/server.ts @@ -23,12 +23,9 @@ import { createSpotifyRouter } from "./api/spotify.js"; import type { SpotifyOAuth } from "../music/spotify/spotify-oauth.js"; import { resolveSpotifyBackendKind } from "../music/spotify/backend-select.js"; import { - isGoLibrespotSupported, - findGoLibrespot, - isRustLibrespotSupported, - findLibrespot, + isGoLibrespotPresent, + isLibrespotPresent, } from "../music/spotify/binary.js"; -import { existsSync } from "node:fs"; import { setupWebSocket } from "./websocket.js"; import { createUserStore } from "../data/users.js"; import { createSessionStore } from "../data/sessions.js"; @@ -156,10 +153,10 @@ export function createWebServer(options: WebServerOptions): WebServer { oauth: options.spotifyOAuth, logger, getBackendInfo: () => { - const goPresent = - isGoLibrespotSupported() && existsSync(findGoLibrespot()); - const rustPresent = - isRustLibrespotSupported() && existsSync(findLibrespot()); + // PATH-aware presence (Bug m1): a PATH-installed binary (bare name) + // is resolved against $PATH, not existsSync()'d against cwd. + const goPresent = isGoLibrespotPresent(); + const rustPresent = isLibrespotPresent(); const resolved = resolveSpotifyBackendKind( options.config.spotify.backend, goPresent,