From 3b42c478bdf975f1f63c7faef204680ff822115b Mon Sep 17 00:00:00 2001 From: "[._.]/ Adam Eivy" <70015+atomantic@users.noreply.github.com> Date: Wed, 2 Sep 2026 06:32:01 +0000 Subject: [PATCH 1/2] fix: find CLIs installed by npm's global prefix, not just the PATH one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Installing the Codex CLI from the AI Providers card succeeded and PortOS still reported "the installer finished, but PortOS still cannot run `codex`. Its bin directory may be missing from PortOS's PATH — restart PortOS, then try again." Restarting never helped. npm writes global binaries to `npm prefix -g`, which is not necessarily a directory the host's Node installer put on PATH. On the reporting machine the prefix is a machine-wide `C:\ProgramData\npm\npm` while PATH still carries only `C:\Program Files\nodejs` and the per-user `%APPDATA%\npm` default — so `codex.cmd` was written somewhere nothing on PATH names, and no restart could inherit a directory the machine PATH never contained. The same gap hits an `NPM_CONFIG_PREFIX` an admin set, an nvm/Volta switch, or a `prefix=` in a user or global npmrc. npm resolves its prefix through a config cascade (cli flags, npm_config_*, project/user/global npmrc, and a builtin npmrc that itself interpolates env vars), so deriving it from %APPDATA%/$HOME guesses would reproduce the bug on the next host. New `server/lib/npmGlobalBin.js` asks npm once and adopts the answer onto `process.env.PATH`, which fixes this process and every child it spawns — the only fix that reaches a TUI provider, which node-pty launches by bare name. Adoption runs at boot in BOTH processes that spawn provider CLIs (the server and the CoS runner, which has its own environment and would otherwise still refuse a CLI the Providers page reports as installed), and again from the runtime probe, because on a first install the prefix directory did not exist when boot looked. The PATH-adoption mechanics move into `processEnv.js` as `adoptPathDirs`, shared with llamaServerManager's winget-shim adoption — which also gains the case-insensitive Windows compare it was missing. The post-install failure message now points at `npm prefix -g` instead of advising a restart that cannot help. --- server/cos-runner/index.js | 8 ++ server/lib/README.md | 4 +- server/lib/index.js | 1 + server/lib/npmGlobalBin.js | 80 +++++++++++++++++++ server/lib/npmGlobalBin.test.js | 74 +++++++++++++++++ server/lib/processEnv.js | 53 +++++++++++- server/lib/processEnv.test.js | 56 ++++++++++++- server/routes/providers.js | 2 +- server/services/bootstrap.js | 10 +++ server/services/llamaServerManager.js | 16 +--- server/services/providerRuntimeInstaller.js | 6 ++ .../services/providerRuntimeInstaller.test.js | 17 ++++ 12 files changed, 309 insertions(+), 18 deletions(-) create mode 100644 server/lib/npmGlobalBin.js create mode 100644 server/lib/npmGlobalBin.test.js 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..420051edd4 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. `pathEntries(env)` splits a PATH into its non-empty entries; `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..396a38d4ad 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,54 @@ export function findCommandOnPath(name, { env = process.env, cwd = process.cwd() } return null; } + +/** + * The PATH entries of an environment, in order, minus the empty segments a + * trailing or doubled delimiter leaves behind. + * + * @param {NodeJS.ProcessEnv|object} [env=process.env] + * @returns {string[]} + */ +export function pathEntries(env = process.env) { + return String(env?.PATH || env?.Path || '').split(delimiter).filter(Boolean); +} + +// 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) { + const entries = pathEntries(); + 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', From 46acf29ddd7a7ef7bf1a697fa86709b08a059618 Mon Sep 17 00:00:00 2001 From: "[._.]/ Adam Eivy" <70015+atomantic@users.noreply.github.com> Date: Wed, 2 Sep 2026 06:34:25 +0000 Subject: [PATCH 2/2] address review: inline the single-use pathEntries helper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It was exported with one call site, and its env?.Path fallback is unreachable — process.env is case-insensitive on Windows, so .PATH already reads a Path entry, and adoptPathDirs was the only caller. --- server/lib/README.md | 2 +- server/lib/processEnv.js | 16 ++++------------ 2 files changed, 5 insertions(+), 13 deletions(-) diff --git a/server/lib/README.md b/server/lib/README.md index 420051edd4..8def09ed6c 100644 --- a/server/lib/README.md +++ b/server/lib/README.md @@ -287,7 +287,7 @@ The barrel `server/lib/index.js` is a machine-checkable enumeration of every pub | `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. | | `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. `pathEntries(env)` splits a PATH into its non-empty entries; `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. | +| `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/processEnv.js b/server/lib/processEnv.js index 396a38d4ad..b3db6194ff 100644 --- a/server/lib/processEnv.js +++ b/server/lib/processEnv.js @@ -209,17 +209,6 @@ export function findCommandOnPath(name, { env = process.env, cwd = process.cwd() return null; } -/** - * The PATH entries of an environment, in order, minus the empty segments a - * trailing or doubled delimiter leaves behind. - * - * @param {NodeJS.ProcessEnv|object} [env=process.env] - * @returns {string[]} - */ -export function pathEntries(env = process.env) { - return String(env?.PATH || env?.Path || '').split(delimiter).filter(Boolean); -} - // 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. @@ -249,7 +238,10 @@ const samePathEntry = (a, b) => (IS_WIN ? a.toLowerCase() === b.toLowerCase() : * @returns {string[]} the directories newly added */ export function adoptPathDirs(dirs) { - const entries = pathEntries(); + // `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;