fix(cli-install): address review: drop every npm_config_prefix spelling, probe async

- Delete every spelling of npm_config_prefix before setting NPM_CONFIG_PREFIX, in the Compose
  branch too: npm run exports the lowercase key and a sorting /bin/sh let it win. The
  operator guard stays on the uppercase key only.
- A prefix that does not exist yet is judged by its nearest existing ancestor.
- The probe is async (promisified execFile, fs.promises.access, SIGKILL on timeout), awaited
  before the spawn and only for commands that run npm.
- Tests: lowercase/mixed-case keys, and the decision logic against a fake npm on PATH
  (writable, read-only, not yet created, npm missing). Docs: one clause in cli-registry.md.

docs/cli-registry.md

@@ -25,7 +25,7 @@ Every run mode Codeman can launch — Claude Code, Terminal/Shell, OpenCode, Cod
 App Settings → Agents & CLIs → **CLI management** (`cliManagementEnabled`, default OFF; admin-only in multi-user mode) lists every entry with an installed/not-installed badge and:
 
 - toggles any entry on or off. A `kind: 'shell'` entry cannot be disabled, and the row shows no switch for it. A disabled CLI disappears from the Run menu, the welcome screen and the phone overview, and new session requests for it are rejected.
-- installs a missing **stock** CLI by running its shipped install command, after a confirm that names the exact command. Only one install per CLI runs at a time, and the command runs without any `CODEMAN_*` variable in its environment. A custom entry's install command is never executed.
+- installs a missing **stock** CLI by running its shipped install command, after a confirm that names the exact command. Only one install per CLI runs at a time, and the command runs without any `CODEMAN_*` variable in its environment. An `npm install -g` command is pointed at `~/.local` (which every resolver searches) when the npm global prefix is not writable by the server user, for example a system node under `/usr`; a writable prefix, a prefix that does not exist yet but could be created, and an explicit `NPM_CONFIG_PREFIX` are left alone. A custom entry's install command is never executed.
 - adds, edits and deletes **custom** entries (id, label, badge, binaries, launch argv). The server re-validates the whole assembled entry through `CliEntrySchema`, so the form cannot bypass the load-time rules.
 
 These are the only writes to `clis.json`. They are serialized, and a file that does not parse or has unsafe permissions is refused rather than overwritten; fix it (or `chmod 600` it) and retry. The HTTP routes are listed in `docs/api-reference.md` under *CLI management*.

src/web/routes/cli-registry-routes.ts

@@ -29,9 +29,11 @@
  * write path (`registry-writer.ts` mirrors `custom-model-hosts.ts`).
  */
 
-import { execFileSync, spawn } from 'node:child_process';
-import { accessSync, constants as fsConstants } from 'node:fs';
-import { join } from 'node:path';
+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 { getAuthUser, isAdmin, parseBody, readJsonConfig, SETTINGS_PATH } from '../route-helpers.js';
@@ -209,31 +211,42 @@ const CLI_INSTALL_TIMEOUT_MS = 300_000;
  */
 const installsInFlight = new Set<string>();
 
+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 function npmGlobalPrefixWritable(source: NodeJS.ProcessEnv): boolean {
+export async function npmGlobalPrefixWritable(source: NodeJS.ProcessEnv): Promise<boolean> {
   try {
-    const prefix = execFileSync('npm', ['config', 'get', 'prefix'], {
+    const { stdout } = await execFileAsync('npm', ['config', 'get', 'prefix'], {
       env: source,
       encoding: 'utf8',
       timeout: 5_000,
-      stdio: ['ignore', 'pipe', 'ignore'],
-    }).trim();
+      killSignal: 'SIGKILL',
+    });
+    const prefix = stdout.trim();
     if (!prefix) return true;
-    // npm creates lib/node_modules under the prefix; check that directory when it exists, else the prefix.
-    for (const dir of [join(prefix, 'lib', 'node_modules'), prefix]) {
+    // 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 {
-        accessSync(dir, fsConstants.W_OK);
+        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;
       }
     }
-    return false;
   } catch {
     return true;
   }
@@ -255,7 +268,7 @@ export function npmGlobalPrefixWritable(source: NodeJS.ProcessEnv): boolean {
  */
 export function installEnv(
   source: NodeJS.ProcessEnv = process.env,
-  prefixWritable: (env: NodeJS.ProcessEnv) => boolean = npmGlobalPrefixWritable
+  prefixWritable: (env: NodeJS.ProcessEnv) => boolean = () => true
 ): NodeJS.ProcessEnv {
   const env: NodeJS.ProcessEnv = {};
   for (const [key, value] of Object.entries(source)) {
@@ -266,13 +279,40 @@ export function installEnv(
   // 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) {
-    env.NPM_CONFIG_PREFIX = `${source.HOME}/.local`;
+    redirectNpmPrefix(env, source.HOME);
   } else if (process.platform !== 'win32' && source.HOME && !source.NPM_CONFIG_PREFIX && !prefixWritable(env)) {
-    env.NPM_CONFIG_PREFIX = `${source.HOME}/.local`;
+    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<NodeJS.ProcessEnv> {
+  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;
@@ -288,6 +328,7 @@ interface InstallResult {
  * CUSTOM entry can never reach this function at all — see the route's own guard below.
  */
 async function runInstallCommand(command: string): Promise<InstallResult> {
+  const env = await installEnvFor(command);
   return new Promise((resolve) => {
     let child: ReturnType<typeof spawn>;
     try {
@@ -299,7 +340,7 @@ async function runInstallCommand(command: string): Promise<InstallResult> {
         // 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: installEnv(),
+        env,
       });
     } catch (err) {
       resolve({ code: null, output: `spawn failed: ${getErrorMessage(err)}`, timedOut: false });

test/routes/cli-registry-routes.test.ts

@@ -12,7 +12,8 @@
  */
 import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
 import { EventEmitter } from 'node:events';
-import { chmodSync, mkdirSync, readFileSync, rmSync, writeFileSync, statSync } from 'node:fs';
+import { chmodSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync, statSync } from 'node:fs';
+import { tmpdir } from 'node:os';
 import { dirname } from 'node:path';
 import { createRouteTestHarness } from './_route-test-utils.js';
 import {
@@ -744,11 +745,70 @@ describe('registry writes are serialized and never clobber a file the reader wou
     expect(installEnv({ PATH: '/usr/bin' }, () => false)).toEqual({ PATH: '/usr/bin' });
   });
 
-  it('npmGlobalPrefixWritable follows npm config and treats a probe failure as writable', () => {
-    // Real probe against this machine: must return a boolean and never throw.
-    expect(typeof npmGlobalPrefixWritable({ PATH: process.env.PATH })).toBe('boolean');
-    // npm not found on PATH: cannot tell, so do not redirect.
-    expect(npmGlobalPrefixWritable({ PATH: '/nonexistent' })).toBe(true);
+  it('drops every spelling of npm_config_prefix when it redirects (npm run exports the lowercase one)', () => {
+    // npm reads npm_config_* case-insensitively and a sorting /bin/sh lets the older key win.
+    expect(installEnv({ HOME: '/h', npm_config_prefix: '/usr' }, () => false)).toEqual({
+      HOME: '/h',
+      NPM_CONFIG_PREFIX: '/h/.local',
+    });
+    expect(installEnv({ HOME: '/h', Npm_Config_Prefix: '/usr', NPM_CONFIG_PREFIX: '' }, () => false)).toEqual({
+      HOME: '/h',
+      NPM_CONFIG_PREFIX: '/h/.local',
+    });
+    expect(
+      installEnv({ CODEMAN_IN_CONTAINER: '1', HOME: '/h', npm_config_prefix: '/usr', NPM_CONFIG_PREFIX: '/opt/x' })
+    ).toEqual({ HOME: '/h', NPM_CONFIG_PREFIX: '/h/.local' });
+    // The operator guard is the uppercase key only: npm run always injects the lowercase one.
+    expect(installEnv({ HOME: '/h', npm_config_prefix: '/usr' }, () => true)).toEqual({
+      HOME: '/h',
+      npm_config_prefix: '/usr',
+    });
+  });
+
+  describe('npmGlobalPrefixWritable', () => {
+    let dir: string;
+    beforeEach(() => {
+      dir = mkdtempSync(`${tmpdir()}/npm-prefix-`);
+    });
+    afterEach(() => {
+      chmodSync(dir, 0o755);
+      rmSync(dir, { recursive: true, force: true });
+    });
+
+    /** A fake `npm` on PATH that prints `prefix` for `npm config get prefix`. */
+    function fakeNpm(prefix: string): NodeJS.ProcessEnv {
+      const bin = `${dir}/bin`;
+      mkdirSync(bin, { recursive: true });
+      writeFileSync(`${bin}/npm`, `#!/bin/sh\necho '${prefix}'\n`, { mode: 0o755 });
+      return { PATH: `${bin}:/usr/bin:/bin` };
+    }
+
+    it('is true for a writable prefix, whether or not lib/node_modules exists', async () => {
+      mkdirSync(`${dir}/w`);
+      expect(await npmGlobalPrefixWritable(fakeNpm(`${dir}/w`))).toBe(true);
+      mkdirSync(`${dir}/w/lib/node_modules`, { recursive: true });
+      expect(await npmGlobalPrefixWritable(fakeNpm(`${dir}/w`))).toBe(true);
+    });
+
+    it('is false for a prefix the server user cannot write', async () => {
+      if (process.getuid?.() === 0) return; // root can write anywhere
+      mkdirSync(`${dir}/ro`);
+      chmodSync(`${dir}/ro`, 0o555);
+      expect(await npmGlobalPrefixWritable(fakeNpm(`${dir}/ro`))).toBe(false);
+    });
+
+    it('judges a prefix that does not exist yet by its nearest existing ancestor', async () => {
+      // A user .npmrc with prefix=~/.npm-global before it was created: npm makes it, so do not move it.
+      expect(await npmGlobalPrefixWritable(fakeNpm(`${dir}/not/yet/made`))).toBe(true);
+      if (process.getuid?.() === 0) return;
+      mkdirSync(`${dir}/ro2`);
+      chmodSync(`${dir}/ro2`, 0o555);
+      expect(await npmGlobalPrefixWritable(fakeNpm(`${dir}/ro2/not/yet`))).toBe(false);
+    });
+
+    it('treats a probe failure (npm missing) as writable, and never throws', async () => {
+      expect(await npmGlobalPrefixWritable({ PATH: '/nonexistent' })).toBe(true);
+    });
   });
 });