diff --git a/server/cos-runner/index.js b/server/cos-runner/index.js index c0631ca75f..fb6e84ca97 100644 --- a/server/cos-runner/index.js +++ b/server/cos-runner/index.js @@ -21,6 +21,7 @@ import { prepareCliSpawn, killProcessTree, guardChildStdin, deliverChildStdin } import { buildCliChildEnv } from '../lib/cliChildEnv.js'; import { prepareCliPrompt } from '../lib/cliProviderArgs.js'; import { commandExists } from '../lib/commandExists.js'; +import { adoptNpmGlobalBinDir } from '../lib/npmGlobalBin.js'; import { findCommandOnPath } from '../lib/processEnv.js'; import { createCodexStderrFormatter } from '../lib/codexCliOutput.js'; import { createStreamJsonParser } from './streamJsonParser.js'; @@ -928,6 +929,13 @@ async function cleanupOrphanedAgents() { server.listen(PORT, HOST, async () => { console.log(`🤖 CoS Agent Runner started on http://${HOST}:${PORT}`); + // The runner is its own PM2 app with its own environment, and it spawns + // provider CLIs by bare name — so it must adopt npm's global bin directory + // itself. Without this a CLI the AI Providers page reports as installed + // (the main server adopted it there) still 422s here as "not on the CoS + // Runner PATH". Fire-and-forget: never blocks accepting work. + adoptNpmGlobalBinDir().catch((err) => console.error(`❌ npm global bin adoption failed: ${err.message}`)); + // Ensure agents directory exists. try/catch is mandatory: this listener runs // outside the request lifecycle, so a rejected await here escapes as an // unhandled rejection and takes the runner down at boot (fatal on Node >= 15). diff --git a/server/lib/README.md b/server/lib/README.md index a37946a3b0..8def09ed6c 100644 --- a/server/lib/README.md +++ b/server/lib/README.md @@ -285,7 +285,9 @@ The barrel `server/lib/index.js` is a machine-checkable enumeration of every pub | `repoUrl.js` | `parseRepoUrl(url)` → `{ host, provider, owner, repo }` / `isRepoUrl(url)` / `repoCloneUrl(parsed)` / `repoBrowseUrl(parsed)` / `parseGitHubUrl(url)` / `isGitHubRepoUrl(url)` — authoritative "is this a clonable repo URL?" rule, with per-host behavior (subgroup nesting, clone layout) in the `REPO_HOSTS` table. Mirrored to `client/src/lib/repoUrl.js` (parity pinned by `repoUrl.mirror.test.js`) so the Brain capture boxes reveal the post-clone agent options for exactly the URLs the server will clone. | | `glabArgs.js` | `GLAB_JSON_ARGS`, `withGlabJson(args)` — pure `glab` argv conventions. The JSON output flag is `--output json`, NOT `-F json`: on `glab issue list` (and only there) `-F` is `--output-format` (details/ids/urls), so `-F json` is accepted, ignored, and answers with the human table at exit 0. Shared by all three `glab` runners so the spelling has one definition; guarded tree-wide by `services/gitlab.glabFlags.test.js`. | | `killWithEscalation.js` | `killWithEscalation(proc, {label, stillRunning, delayMs=8000})` — shared SIGTERM→grace→SIGKILL cancel-escalation for spawn-based media jobs. Sends SIGTERM, then escalates to SIGKILL after `delayMs` only when `stillRunning()` holds and the child hasn't exited (`exitCode===null && signalCode===null`). The timer is unref'd and the callback is try/catch-wrapped (runs outside the request lifecycle). Converges musicVideo/render, videoTimeline, imageGen local+codex, videoGen, loraTraining, and the yt-dlp track import cancel paths. | -| `processEnv.js` | `stripDebugMallocEnv(env)` / `safeChildProcessEnv(extra)` — drop macOS `Malloc*` debug env vars before spawning a child. `safeChildProcessOptions(options)` adds that sanitized environment plus `windowsHide: true`, preventing detached PM2 subprocesses from opening transient Windows console UI; `buildSafeCliBaseEnv(env, provider)` allowlists runtime/delivery essentials and only the selected CLI's ambient provider auth before an AI CLI inherits the server environment. Route Node→Python and other background CLI spawns through the process helpers. Also `whichFirst(name)` — first-hit `which`/`where` PATH probe (safe options, 5s timeout) returning the absolute binary path or `null`; `whichFirstSync(name)` provides the same contract without the await; `findCommandOnPath(name, { env, cwd })` resolves an executable from the exact child environment without needing `which` itself on that PATH. | +| `npmGlobalBin.js` | `adoptNpmGlobalBinDir()` — puts the directory `npm install --global` actually writes to onto `process.env.PATH`, from one cached `npm prefix -g`. npm resolves its prefix through a config cascade (cli flags, `npm_config_*`, project/user/global npmrc, a builtin npmrc that interpolates env vars), so guessing it from `%APPDATA%`/`$HOME` reproduces the bug on the next host — only npm can answer. Exists because a host whose npm prefix is NOT the directory its Node installer put on PATH (a machine-wide Windows prefix vs the per-user `%APPDATA% +pm` default, `NPM_CONFIG_PREFIX`, nvm/Volta) installed `codex` successfully and then reported it unrunnable, advising a restart that inherits the same PATH. Called at boot in BOTH processes that spawn provider CLIs (`services/bootstrap.js`, `cos-runner/index.js`) and again from `providerRuntimeInstaller.js`'s probe, because on a first install the prefix directory did not exist when boot looked. | +| `processEnv.js` | `stripDebugMallocEnv(env)` / `safeChildProcessEnv(extra)` — drop macOS `Malloc*` debug env vars before spawning a child. `safeChildProcessOptions(options)` adds that sanitized environment plus `windowsHide: true`, preventing detached PM2 subprocesses from opening transient Windows console UI; `buildSafeCliBaseEnv(env, provider)` allowlists runtime/delivery essentials and only the selected CLI's ambient provider auth before an AI CLI inherits the server environment. Route Node→Python and other background CLI spawns through the process helpers. Also `whichFirst(name)` — first-hit `which`/`where` PATH probe (safe options, 5s timeout) returning the absolute binary path or `null`; `whichFirstSync(name)` provides the same contract without the await; `findCommandOnPath(name, { env, cwd })` resolves an executable from the exact child environment without needing `which` itself on that PATH. `adoptPathDirs(dirs)` appends the ones that exist and are not already there to THIS process's `process.env.PATH` (case-insensitively on Windows), which fixes this process AND every child, since the env builders above derive from `process.env` — the only fix that reaches a CLI launched by BARE NAME through node-pty. Shared by `npmGlobalBin.js` and `services/llamaServerManager.js`'s winget-shim adoption. | | `branchUpstreamGuard.js` | `enforceSafeBranchUpstream(repo, branch)` / `readBranchUpstream` / `isSafeBranchUpstream` — the agent-branch upstream invariant (#4172): a branch handed to a CoS agent tracks either NOTHING or its OWN ref, never `refs/heads/main`. `git worktree add -b origin/main` does not leave the branch untracked — `branch.autoSetupMerge` records `merge=refs/heads/main` — and `/do:pr` derives its push destination from that config (`git push HEAD:`), so the agent's work lands straight on the default branch with no PR. Prevention is `--no-track` at the `worktreeManager.js` creation sites; this is the backstop that also REPAIRS branches created before the fix (drops the bogus upstream, logs it) and throws only if the repair doesn't take. Fails CLOSED: `readBranchUpstream` returns `null` for could-not-read (distinct from `''` = genuinely unset), and an unverifiable upstream is refused rather than waved through — `worktreeManager.js` undoes the `worktree add` on refusal so the throw can't strand a tree. Pairs with `primaryCheckoutGuard.js`, which catches the other way agent work reaches `main`. | | `primaryCheckoutGuard.js` | `capturePrimaryCheckoutState(path)` / `detectPrimaryCheckoutDrift(baseline, {agentBranch})` + `PRIMARY_CHECKOUT_MUTATED_REASON`/`_CATEGORY` — the branch-jack detector (#3680): stamps the PRIMARY checkout's branch + HEAD onto a worktree agent's metadata at spawn (`agentLifecycle.js`) and re-reads it in the shared finalize path (`agentFinalization.js`), so a worktree-isolated agent that commits to the primary is recorded as a FAILURE naming the drifted branch, the commit count, and the `git reset --hard` recovery — instead of a silent "completed". Detect-and-report only: the reset discards commits, so it stays a human decision. Non-throwing (runs outside the request lifecycle); an unreadable checkout reports no drift rather than inventing one. | | `pythonSetup.js` | Python venv / runner setup helpers. | diff --git a/server/lib/index.js b/server/lib/index.js index bf1bf97255..3b44d38ac6 100644 --- a/server/lib/index.js +++ b/server/lib/index.js @@ -269,6 +269,7 @@ export * from './glabArgs.js'; export * from './goalFeatureMap.js'; export * from './interactiveShellResolver.js'; export * from './killWithEscalation.js'; +export * from './npmGlobalBin.js'; export * from './openFolder.js'; export * from './processEnv.js'; export * from './primaryCheckoutGuard.js'; diff --git a/server/lib/npmGlobalBin.js b/server/lib/npmGlobalBin.js new file mode 100644 index 0000000000..8fe92b2ee1 --- /dev/null +++ b/server/lib/npmGlobalBin.js @@ -0,0 +1,80 @@ +/** + * Where `npm install --global` actually writes its executables. + * + * The AI Providers page installs a CLI with a fixed `npm install --global` and + * then asks whether PortOS can run it. Those two questions have different + * answers on any host whose npm prefix is not the directory the platform's Node + * installer put on PATH: a machine-wide Windows prefix while PATH still carries + * the per-user `%APPDATA%\npm` default, an `NPM_CONFIG_PREFIX` an admin set, an + * nvm/Volta switch, a `prefix=` in a user or global npmrc. The install writes a + * perfectly good `codex.cmd` and nothing on PATH names the directory holding + * it — so the card reported "the installer finished, but PortOS still cannot + * run `codex`" and advised a restart, which cannot fix a directory the machine + * PATH has never contained. + * + * npm resolves its prefix through a config cascade (cli flags, `npm_config_*` + * env, project/user/global npmrc, and a builtin npmrc that itself interpolates + * env vars), so re-deriving it from `%APPDATA%`/`$HOME` guesses would just + * reproduce the same class of bug on the next host. The only correct answer is + * npm's own, hence one cached `npm prefix -g`. + * + * `adoptPathDirs` does the rest; see its docstring in `processEnv.js` for why + * extending `process.env.PATH` is what a bare-name node-pty launch needs. + */ + +import { existsSync } from 'fs'; +import { join } from 'path'; +import { bufferedSpawn, prepareCliSpawn } from './bufferedSpawn.js'; +import { adoptPathDirs, safeChildProcessEnv } from './processEnv.js'; + +const IS_WIN = process.platform === 'win32'; + +// `npm prefix -g` reads config files and can pay a cold Node start, so it gets +// more room than the 5s PATH probes — but it must never hang a status refresh. +const PREFIX_TIMEOUT_MS = 15_000; + +// npm's answer, held as a promise so concurrent callers share one spawn: a host +// does not change its global prefix underneath a running server. +let binDirProbe = null; + +/** One `npm prefix -g`, mapped to the platform's global bin directory. */ +async function probeNpmGlobalBinDir(spawnImpl) { + const env = safeChildProcessEnv(); + // `shell: false` + `prepareCliSpawn`, not `bufferedSpawn`'s `needsShell('npm')` + // default: that default is `shell: true` with an args array, which node + // space-joins WITHOUT escaping (DEP0190). The argv here is fixed, but the + // warning would print at every boot in both processes. + const { command, args } = prepareCliSpawn('npm', ['prefix', '-g'], env); + const { stdout } = await spawnImpl(command, args, { env, shell: false, timeoutMs: PREFIX_TIMEOUT_MS }); + const prefix = String(stdout || '').trim().split(/\r?\n/)[0]?.trim() || ''; + // Windows drops binaries straight in the prefix; POSIX uses `/bin`. + return prefix ? (IS_WIN ? prefix : join(prefix, 'bin')) : null; +} + +/** + * Put npm's global bin directory on this process's PATH, so a CLI installed + * there is both discoverable and launchable by bare name. Idempotent, and a + * no-op when npm cannot be asked or the directory is not there yet. + * + * The existence check is deliberately NOT cached with npm's answer: the prefix + * directory does not exist until the first global install, and making that + * install visible without a restart is the whole point. + * + * @param {{spawnImpl?: Function}} [deps] + * @returns {Promise} the directory now on PATH, or `null` + */ +export async function adoptNpmGlobalBinDir({ spawnImpl = bufferedSpawn } = {}) { + // A probe that cannot answer resolves null rather than rejecting: an + // unreachable npm means "PortOS keeps the PATH it has", not a failed caller. + binDirProbe = binDirProbe || probeNpmGlobalBinDir(spawnImpl).catch(() => null); + const dir = await binDirProbe; + if (!dir || !existsSync(dir)) return null; + const [adopted] = adoptPathDirs([dir]); + if (adopted) console.log(`🔗 Adopted npm global bin directory onto PortOS's PATH: ${adopted}`); + return dir; +} + +/** Test-only: forget npm's answer so the next read re-probes. */ +export function __resetNpmGlobalBinCache() { + binDirProbe = null; +} diff --git a/server/lib/npmGlobalBin.test.js b/server/lib/npmGlobalBin.test.js new file mode 100644 index 0000000000..b41ac99e3e --- /dev/null +++ b/server/lib/npmGlobalBin.test.js @@ -0,0 +1,74 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { mkdtempSync, mkdirSync, rmSync } from 'fs'; +import { tmpdir } from 'os'; +import { delimiter, join } from 'path'; +import { adoptNpmGlobalBinDir, __resetNpmGlobalBinCache } from './npmGlobalBin.js'; + +const IS_WIN = process.platform === 'win32'; + +// npm answers with the PREFIX; the bin directory is the prefix itself on +// Windows and `/bin` everywhere else. +const binDirFor = (prefix) => (IS_WIN ? prefix : join(prefix, 'bin')); +const npmAnswering = (prefix) => vi.fn(async () => ({ stdout: `${prefix}\n` })); + +describe('adoptNpmGlobalBinDir', () => { + let root; + let originalPath; + + beforeEach(() => { + __resetNpmGlobalBinCache(); + root = mkdtempSync(join(tmpdir(), 'portos-npm-prefix-')); + originalPath = process.env.PATH; + }); + + afterEach(() => { + process.env.PATH = originalPath; + rmSync(root, { recursive: true, force: true }); + }); + + it('puts npm\'s own global bin directory on PATH, once, for concurrent callers', async () => { + const binDir = binDirFor(root); + mkdirSync(binDir, { recursive: true }); + const spawnImpl = npmAnswering(root); + + const both = await Promise.all([ + adoptNpmGlobalBinDir({ spawnImpl }), + adoptNpmGlobalBinDir({ spawnImpl }), + ]); + + expect(both).toEqual([binDir, binDir]); + expect(process.env.PATH.split(delimiter).filter((entry) => entry === binDir)).toHaveLength(1); + // The Providers page probes every runtime at once, and each one asks. + expect(spawnImpl).toHaveBeenCalledTimes(1); + // Windows wraps npm's .cmd shim as `cmd.exe /c …` — the fixed + // subcommand is the tail either way, and never a shell string. + const [, args, options] = spawnImpl.mock.calls[0]; + expect(args.slice(-2)).toEqual(['prefix', '-g']); + expect(options.shell).toBe(false); + }); + + it('answers null when npm cannot be asked, without falling back to a guess', async () => { + const spawnImpl = vi.fn(async () => { throw new Error('ENOENT'); }); + + await expect(adoptNpmGlobalBinDir({ spawnImpl })).resolves.toBeNull(); + expect(process.env.PATH).toBe(originalPath); + }); + + // The prefix directory does not exist until the first global install, and + // making that install visible without a restart is the whole point — so the + // on-disk check must NOT be cached alongside npm's answer. + it('adopts a prefix that only appears after the first global install', async () => { + const binDir = binDirFor(root); + rmSync(root, { recursive: true, force: true }); + const spawnImpl = npmAnswering(root); + + await expect(adoptNpmGlobalBinDir({ spawnImpl })).resolves.toBeNull(); + expect(process.env.PATH).toBe(originalPath); + + mkdirSync(binDir, { recursive: true }); + await expect(adoptNpmGlobalBinDir({ spawnImpl })).resolves.toBe(binDir); + expect(process.env.PATH.split(delimiter)).toContain(binDir); + // Still one probe: only the existence check re-runs. + expect(spawnImpl).toHaveBeenCalledTimes(1); + }); +}); diff --git a/server/lib/processEnv.js b/server/lib/processEnv.js index 47050b87f1..b3db6194ff 100644 --- a/server/lib/processEnv.js +++ b/server/lib/processEnv.js @@ -12,7 +12,7 @@ // stripping the prefix is a no-op on Linux/Windows. import { execFile, execFileSync } from './childProcess.js'; import { promisify } from 'util'; -import { accessSync, constants, statSync } from 'fs'; +import { accessSync, constants, existsSync, statSync } from 'fs'; import { delimiter, isAbsolute, join, resolve } from 'path'; const execFileAsync = promisify(execFile); @@ -208,3 +208,46 @@ export function findCommandOnPath(name, { env = process.env, cwd = process.cwd() } return null; } + +// Windows paths are case-insensitive and PATH mixes casings freely +// (`C:\ProgramData\npm` vs `c:\programdata\npm`), so a case-sensitive compare +// would append a duplicate of an entry that is already there. +const samePathEntry = (a, b) => (IS_WIN ? a.toLowerCase() === b.toLowerCase() : a === b); + +/** + * Adopt directories onto THIS process's PATH. + * + * A package manager that installs into a directory the running process's PATH + * does not name is the recurring case, and it always looks the same from here: + * the install succeeds and the binary reports as "not found". winget adds its + * portable-shim Links directory to the USER environment, which an + * already-running server never inherited; npm's global prefix need not be the + * directory the platform's Node installer put on PATH at all, so no restart + * fixes that one. + * + * Extending `process.env.PATH` fixes both this process's own lookups AND every + * child it spawns, since `safeChildProcessEnv` / `buildSafeCliBaseEnv` derive + * from it — which is what a CLI launched by BARE NAME through node-pty needs, + * because resolving its path is not something that launch can use. + * + * Only directories that exist are adopted: `findCommandOnPath` stats every PATH + * entry against every PATHEXT extension, so a dead entry taxes every later + * lookup for the life of the process. + * + * @param {string[]} dirs + * @returns {string[]} the directories newly added + */ +export function adoptPathDirs(dirs) { + // `process.env` is case-insensitive on Windows, so `.PATH` reads a `Path` + // entry too — no second spelling to check. The empty segments a trailing or + // doubled delimiter leaves behind are dropped rather than carried along. + const entries = String(process.env.PATH || '').split(delimiter).filter(Boolean); + const added = []; + for (const dir of dirs) { + if (!dir || entries.some((entry) => samePathEntry(entry, dir)) || !existsSync(dir)) continue; + entries.push(dir); + added.push(dir); + } + if (added.length > 0) process.env.PATH = entries.join(delimiter); + return added; +} diff --git a/server/lib/processEnv.test.js b/server/lib/processEnv.test.js index 81201df663..4aabbd010d 100644 --- a/server/lib/processEnv.test.js +++ b/server/lib/processEnv.test.js @@ -1,6 +1,8 @@ -import { describe, it, expect } from 'vitest'; -import { basename, dirname } from 'path'; -import { buildSafeCliBaseEnv, findCommandOnPath, safeChildProcessEnv, stripDebugMallocEnv, whichFirst } from './processEnv.js'; +import { afterEach, beforeEach, describe, it, expect } from 'vitest'; +import { mkdtempSync, rmSync } from 'fs'; +import { tmpdir } from 'os'; +import { basename, delimiter, dirname, join } from 'path'; +import { adoptPathDirs, buildSafeCliBaseEnv, findCommandOnPath, safeChildProcessEnv, stripDebugMallocEnv, whichFirst } from './processEnv.js'; describe('stripDebugMallocEnv', () => { it('drops every key that starts with "Malloc"', () => { @@ -154,3 +156,51 @@ describe('findCommandOnPath', () => { expect(resolved).toBe(nodePath); }); }); + +describe('adoptPathDirs', () => { + const IS_WIN = process.platform === 'win32'; + let root; + let originalPath; + + beforeEach(() => { + root = mkdtempSync(join(tmpdir(), 'portos-adopt-path-')); + originalPath = process.env.PATH; + }); + + afterEach(() => { + process.env.PATH = originalPath; + rmSync(root, { recursive: true, force: true }); + }); + + // A package manager that installed into a directory this process's PATH does + // not name (winget's Links dir, npm's global prefix) is invisible to both + // findCommandOnPath AND the bare-name spawns children make until it is here. + it('appends a real directory and reports it, skipping one that does not exist', () => { + const missing = join(root, 'never-created'); + + expect(adoptPathDirs([root, missing])).toEqual([root]); + + const entries = process.env.PATH.split(delimiter); + expect(entries).toContain(root); + // A dead entry taxes every later PATH walk, so it must never be added. + expect(entries).not.toContain(missing); + }); + + it('does not re-add a directory PATH already carries', () => { + adoptPathDirs([root]); + const afterFirst = process.env.PATH; + + expect(adoptPathDirs([root, root])).toEqual([]); + expect(process.env.PATH).toBe(afterFirst); + }); + + // Windows PATH mixes casings freely and its paths are case-insensitive, so a + // case-sensitive compare would append a duplicate of an entry already there. + it.runIf(IS_WIN)('matches an existing Windows PATH entry regardless of case', () => { + process.env.PATH = `${originalPath}${delimiter}${root.toUpperCase()}`; + const before = process.env.PATH; + + expect(adoptPathDirs([root])).toEqual([]); + expect(process.env.PATH).toBe(before); + }); +}); diff --git a/server/routes/providers.js b/server/routes/providers.js index 0675e2fc18..12397706ec 100644 --- a/server/routes/providers.js +++ b/server/routes/providers.js @@ -442,7 +442,7 @@ export function createPortOSProviderRoutes(aiToolkit) { if (installed) { emit({ type: 'complete', message: `${runtime.label} is installed and available to PortOS.` }); } else if (code === 0) { - emit({ type: 'error', message: `The installer finished, but PortOS still cannot run \`${runtime.command}\`. Its bin directory may be missing from PortOS's PATH — restart PortOS, then try again.` }); + emit({ type: 'error', message: `The installer finished, but PortOS still cannot run \`${runtime.command}\`. npm wrote it to a bin directory that is not on this machine's PATH — run \`npm prefix -g\` in a terminal, add that directory (plus \`/bin\` off Windows) to your PATH, then restart PortOS.` }); } else { emit({ type: 'error', message: `${runtime.label} installer exited with code ${code}.` }); } diff --git a/server/services/bootstrap.js b/server/services/bootstrap.js index 22302f2e8c..221896ad3b 100644 --- a/server/services/bootstrap.js +++ b/server/services/bootstrap.js @@ -32,6 +32,7 @@ import { ERROR_CATEGORIES } from '../lib/aiToolkit/errorDetection.js'; import { createAIToolkit } from '../lib/aiToolkit/index.js'; import { verifyCollectionVersions } from '../lib/collectionStore.js'; import { startIdleReaper, stopIdleReaper } from '../lib/managedDaemon.js'; +import { adoptNpmGlobalBinDir } from '../lib/npmGlobalBin.js'; import { conflictJournalStore } from '../lib/conflictJournal.js'; import { markHostShuttingDown, writeHostShutdownMarker } from '../lib/hostShutdown.js'; import { setUserCatalogTypes } from '../lib/catalogTypes.js'; @@ -316,6 +317,15 @@ export const bootstrapServices = async ({ io, dataDir, dataReferenceDir, serverD * server from listening; each logs its own failure and the boot continues. */ const startBackgroundServices = ({ spawnerReady, io }) => { + // Put npm's global bin directory on PATH before anything spawns a provider + // CLI. npm's prefix need not be the directory the host's Node installer put + // on PATH, and a CLI installed there is invisible to the bare-name spawn a + // TUI provider uses until it is adopted. One `npm prefix -g` child, never an + // AI provider call — safe under AGENTS.md's no-cold-bootstrap rule, on the + // same footing as the `--version` probes providerPrerequisites.js already + // runs. The CoS runner is a separate process and adopts it separately. + adoptNpmGlobalBinDir().catch((err) => console.error(`❌ npm global bin adoption failed: ${err.message}`)); + // Explicit call (not a module-level side effect) so test imports of cos.js // don't spin up its event listeners and timers. The spawner gate itself lives // in bootstrapSequence.js. diff --git a/server/services/llamaServerManager.js b/server/services/llamaServerManager.js index 6403dbaecb..e72f52d24f 100644 --- a/server/services/llamaServerManager.js +++ b/server/services/llamaServerManager.js @@ -7,11 +7,9 @@ */ import { realpath, stat } from 'fs/promises'; -import { existsSync } from 'fs'; -import { delimiter } from 'path'; import { spawn } from '../lib/childProcess.js'; import { commandExists } from '../lib/commandExists.js'; -import { findCommandOnPath, safeChildProcessEnv, safeChildProcessOptions } from '../lib/processEnv.js'; +import { adoptPathDirs, findCommandOnPath, safeChildProcessEnv, safeChildProcessOptions } from '../lib/processEnv.js'; import { expandHome, sleep } from '../lib/fileUtils.js'; import { createDaemonWatcher, pm2ArgValue, LLAMA_APP } from '../lib/managedDaemon.js'; import { execFile } from '../lib/childProcess.js'; @@ -1434,19 +1432,13 @@ function linkLlamaCpp(env) { * winget links a portable package's executables into a Links directory and adds * that directory to the USER environment — a change an already-running PortOS * does not inherit, so a perfectly successful install would otherwise report - * "llama-server was not found on PATH" until the server was restarted. Extending - * `process.env.PATH` fixes both this process's own lookups and every child it - * spawns, since `safeChildProcessEnv` derives from `process.env`. + * "llama-server was not found on PATH" until the server was restarted. See + * `adoptPathDirs` for why extending `process.env.PATH` is the fix. * * @returns {string|null} the newly-visible binary path, or null if it is not there */ function adoptWingetLinkDirs() { - const existing = (process.env.PATH || process.env.Path || '').split(delimiter).filter(Boolean); - // Only directories that are actually there: `findCommandOnPath` walks every - // PATH entry against every PATHEXT extension with a synchronous stat, so a - // dead entry taxes every later lookup for the life of the process. - const missing = wingetLinkDirs().filter((dir) => !existing.includes(dir) && existsSync(dir)); - if (missing.length > 0) process.env.PATH = [...existing, ...missing].join(delimiter); + adoptPathDirs(wingetLinkDirs()); return resolveLlamaServerBinary(); } diff --git a/server/services/providerRuntimeInstaller.js b/server/services/providerRuntimeInstaller.js index a311eb3ee4..27a8ab17ab 100644 --- a/server/services/providerRuntimeInstaller.js +++ b/server/services/providerRuntimeInstaller.js @@ -38,6 +38,7 @@ import { spawn } from '../lib/childProcess.js'; import { killProcessTree, prepareCliSpawn } from '../lib/bufferedSpawn.js'; import { commandExists } from '../lib/commandExists.js'; +import { adoptNpmGlobalBinDir } from '../lib/npmGlobalBin.js'; import { findCommandOnPath, safeChildProcessEnv, safeChildProcessOptions } from '../lib/processEnv.js'; import { PROVIDER_VENDORS } from '../lib/providerVendors.js'; @@ -177,6 +178,11 @@ async function probeRuntimeStatus(runtime, findCommand, probeCommand) { const kind = runtime.install.kind; const tool = INSTALL_TOOL[kind]; + // Boot adopts this already; repeat it here because the install route probes + // again straight after `npm install --global`, and on a first install the + // prefix directory did not exist when boot looked. Cached and idempotent. + await adoptNpmGlobalBinDir(); + const [resolved, toolPath] = await Promise.all([findCommand(runtime.command), findCommand(tool)]); // `where ` can select npm's extensionless POSIX shim before the working diff --git a/server/services/providerRuntimeInstaller.test.js b/server/services/providerRuntimeInstaller.test.js index 9de2586db4..a5bbb7056c 100644 --- a/server/services/providerRuntimeInstaller.test.js +++ b/server/services/providerRuntimeInstaller.test.js @@ -1,5 +1,11 @@ import { beforeEach, describe, expect, it, vi } from 'vitest'; import { PROVIDER_VENDORS } from '../lib/providerVendors.js'; + +// The npm-prefix probe shells out; the PATH-adoption contract itself is covered +// by lib/npmGlobalBin.test.js. +const npmGlobalBin = vi.hoisted(() => ({ adoptNpmGlobalBinDir: vi.fn(async () => null) })); +vi.mock('../lib/npmGlobalBin.js', () => npmGlobalBin); + import { buildRuntimeInstallCommand, getProviderRuntime, @@ -17,6 +23,7 @@ const IS_WIN = process.platform === 'win32'; describe('provider runtime installer', () => { beforeEach(() => { __resetRuntimeStatusCache(); + npmGlobalBin.adoptNpmGlobalBinDir.mockClear(); }); it('reports runnable availability as booleans without returning local paths', async () => { @@ -50,6 +57,16 @@ describe('provider runtime installer', () => { expect(probeCommand).toHaveBeenCalledWith('/example/codex', ['--version'], { timeoutMs: 15_000 }); }); + // npm installs into ITS OWN global bin directory, which is not necessarily + // one the host's Node installer put on PATH. Without adopting that directory + // the probe reports a perfectly installed CLI as missing, and the card offers + // an install that can never take. + it('adopts the npm global bin directory before probing', async () => { + await getProviderRuntimeStatus('codex', { findCommand: async () => null, probeCommand: async () => false }); + + expect(npmGlobalBin.adoptNpmGlobalBinDir).toHaveBeenCalled(); + }); + it('reports a PATH-resolved but broken CLI as unavailable', async () => { const status = await getProviderRuntimeStatus('codex', { findCommand: async () => '/example/codex',