diff --git a/desktop/docs/frontend-api.md b/desktop/docs/frontend-api.md index 63037d09..caacf570 100644 --- a/desktop/docs/frontend-api.md +++ b/desktop/docs/frontend-api.md @@ -117,6 +117,15 @@ environment assignments pass through without an engine-option catalog. - `deleteModel`; - `setModelExpiry`. +`install` and `uninstall` carry `path`, the user's answer to changing their +PATH. The renderer asks before a local install (once for every engine the +first-run wizard installs) and offers removal on uninstall only when the local +status reports `pathManaged`; otherwise the uninstall omits `path`, which keeps +PAIR's claim on any entry instead of handing it to the user unasked. Remote +installs never change PATH. `update` reads `path_managed` fresh from +`engine:status`, since `nvpair-tui` can change it without a push, and reuses +that answer without asking. + Commands return no state. Renderer stores update from `engines:state-changed`, `engines:progress-changed`, and `engines:progress-cleared`. diff --git a/desktop/docs/services-api.md b/desktop/docs/services-api.md index 7aea40f9..3d2f27da 100644 --- a/desktop/docs/services-api.md +++ b/desktop/docs/services-api.md @@ -232,9 +232,11 @@ |---|---|---| | `cluster:identity-changed` | request (we call) | ✅ yes | | `cluster:invite-received` | request (we call) | ✅ yes | +| `engine:install` | request (we call) | ✅ yes | | `engine:install-progress` | request (we call) | ✅ yes | | `engine:pull-progress` | request (we call) | ✅ yes | | `engine:state-changed` | request (we call) | ✅ yes | +| `engine:uninstall` | request (we call) | ✅ yes | | `error` | request (we call) | ✅ yes | | `nodes:changed` | request (we call) | ✅ yes | | `workloads:remove` | request (we call) | ✅ yes | diff --git a/desktop/docs/services-parity.md b/desktop/docs/services-parity.md index c0b8cbc5..d5ec8610 100644 --- a/desktop/docs/services-parity.md +++ b/desktop/docs/services-parity.md @@ -177,7 +177,8 @@ sign the OFF choice was discarded. For ease of use, Personal AI Router issues starts in two additional cases and keeps no auto-start list of its own: every install sends `engine:install` with -`start: true`, so a successful install (or managed update) starts the engine; +`start: true` (and `path` set to the user's PATH answer), so a successful +install (or managed update) starts the engine; and on the first app open Personal AI Router starts every already-installed local engine once. Both paths rely on the backend recording desired-enabled as a side effect of start, so `nvpair-engine-manager` remains the single owner of diff --git a/desktop/electron-builder.config.ts b/desktop/electron-builder.config.ts index 1f713d47..2407e9e8 100644 --- a/desktop/electron-builder.config.ts +++ b/desktop/electron-builder.config.ts @@ -10,6 +10,7 @@ // signing and notarization live outside this repository, so anything built here // is unsigned. Released builds come from NVIDIA's own signed pipeline. import { readFileSync, readdirSync } from 'node:fs' +import { join } from 'node:path' import type { Configuration } from 'electron-builder' import electronPkg from 'electron/package.json' import pkg from './package.json' @@ -390,7 +391,15 @@ const config: Configuration = { }, deb: { afterInstall: 'scripts/build/linux/after-install.sh', - afterRemove: 'scripts/build/linux/after-remove.sh' + afterRemove: 'scripts/build/linux/after-remove.sh', + // prerm has no dedicated option, so it goes through the raw fpm passthrough. + // It keeps a copy of the engine-manager for the purge-time PATH cleanup in + // after-remove, because dpkg deletes the package's files before postrm + // runs — see before-remove.sh. Unlike afterInstall/afterRemove, fpm arguments are + // forwarded verbatim: no ${macro} expansion, and the path is resolved + // against fpm's working directory rather than this file, so pass an + // absolute one. + fpm: [`--before-remove=${join(__dirname, 'scripts/build/linux/before-remove.sh')}`] }, mac: { executableName: APP_EXECUTABLE_NAME, diff --git a/desktop/scripts/build/installer.nsh b/desktop/scripts/build/installer.nsh index 1517393e..eba4a41f 100644 --- a/desktop/scripts/build/installer.nsh +++ b/desktop/scripts/build/installer.nsh @@ -78,6 +78,45 @@ ClearErrors !macroend +; Release the PATH entries this user's engines own, before the data directory +; that records them is deleted. +; +; Engine-manager writes its ownership receipts under the data root +; (engine-bin\engine-path\), while the entries themselves live in +; HKCU\Environment\Path — outside everything pairRemoveUserData touches. Deleting +; the receipts first would strand those entries with no record left to remove +; them by, pointing at engine directories this uninstall is about to delete. +; +; Runs the shipped binary rather than editing the registry here, so one +; implementation owns the format and the ownership rules. It must run before the +; template's RMDir /r $INSTDIR, which is why this lives in customUnInstall. +; nsExec::ExecToLog never aborts the uninstaller. +; +; The exit code is kept in $7 because the caller has to act on it. A failed +; release is not a no-op: the binary removes what it can and reports the rest, +; so some entries may be gone and some may remain — and the records that could +; still identify the remaining ones are inside the data root the caller is about +; to delete. +; +; The child inherits this process's environment and user, so it resolves the +; same profile pairRemoveUserData does. An elevated uninstall authenticated as a +; different administrator targets that account instead — the same limitation the +; data removal above already has. +!macro pairReleaseEnginePathEntries + DetailPrint "Releasing engine PATH entries..." + nsExec::ExecToLog '"$INSTDIR\resources\cli-bin\nvpair-engine-manager.exe" --remove-user-path' + Pop $7 +!macroend + +; The release failed, so the ownership records are the only thing that can still +; identify the entries it left behind. Deleting the data root would strand them +; permanently — the outcome the whole ordering above exists to avoid — so the +; data stays and the user is told why. A reinstall retries the cleanup. +!macro pairWarnPathEntriesRemain + DetailPrint "Could not release every engine PATH entry; keeping user data so a reinstall can retry." + MessageBox MB_OK|MB_ICONEXCLAMATION "Personal AI Router could not remove every engine entry from your PATH.$\n$\nYour data has been kept so that reinstalling can finish the cleanup. If you delete it by hand, remove those PATH entries yourself as well — nothing else will be able to identify them." /SD IDOK +!macroend + ; Best-effort: when the user opts to remove data, stop any process whose ; executable lives UNDER one of the data roots (e.g. an engine like Ollama ; running from %LOCALAPPDATA%\Nvidia Corporation\Personal AI Router\engine-bin\) @@ -314,9 +353,14 @@ pairDataDone: ${endif} ${if} $8 == "1" - !insertmacro pairKillProcessesInDataDirs - !insertmacro pairRemoveUserData - !insertmacro pairWarnIfDataRemains + !insertmacro pairReleaseEnginePathEntries + ${if} $7 == "0" + !insertmacro pairKillProcessesInDataDirs + !insertmacro pairRemoveUserData + !insertmacro pairWarnIfDataRemains + ${else} + !insertmacro pairWarnPathEntriesRemain + ${endif} ${endif} ${endif} !macroend diff --git a/desktop/scripts/build/linux/after-remove.sh b/desktop/scripts/build/linux/after-remove.sh index f506a05d..721a4274 100644 --- a/desktop/scripts/build/linux/after-remove.sh +++ b/desktop/scripts/build/linux/after-remove.sh @@ -60,7 +60,28 @@ case "${1:-}" in real_user="$(logname 2>/dev/null || true)" fi - if [ -n "$real_user" ]; then + # before-remove.sh kept this copy of nvpair-engine-manager, because the + # package's own is gone by now. It releases the PATH entries this user's + # engines own, which live in the login shell profiles rather than the data + # root, so they have to go before the records that identify them. A failed + # release keeps the data root, so the records survive for a later attempt. + stash_dir="/var/lib/$DPKG_MAINTSCRIPT_PACKAGE" + keep_data=0 + if [ -n "$real_user" ] && [ "$real_user" != root ] && [ -n "$DPKG_MAINTSCRIPT_PACKAGE" ] \ + && [ -x "$stash_dir/nvpair-engine-manager" ]; then + if command -v runuser >/dev/null 2>&1; then + runuser -u "$real_user" -- "$stash_dir/nvpair-engine-manager" --remove-user-path >/dev/null 2>&1 || keep_data=1 + else + su -s /bin/sh -c "'$stash_dir/nvpair-engine-manager' --remove-user-path" "$real_user" >/dev/null 2>&1 || keep_data=1 + fi + fi + if [ -n "$DPKG_MAINTSCRIPT_PACKAGE" ]; then + rm -rf "$stash_dir" 2>/dev/null || true + fi + + if [ -n "$real_user" ] && [ "$keep_data" = 1 ]; then + echo "Personal AI Router: could not remove the PATH entries its engines added; keeping its data so they can still be removed after reinstalling." >&2 + elif [ -n "$real_user" ]; then user_home="$(getent passwd "$real_user" 2>/dev/null | cut -d: -f6 || true)" [ -n "$user_home" ] || user_home="/home/$real_user" diff --git a/desktop/scripts/build/linux/before-remove.sh b/desktop/scripts/build/linux/before-remove.sh new file mode 100644 index 00000000..6daa4efd --- /dev/null +++ b/desktop/scripts/build/linux/before-remove.sh @@ -0,0 +1,47 @@ +#!/bin/bash +# SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 + +# Personal AI Router Debian pre-remove. Keeps a copy of nvpair-engine-manager so +# that after-remove can release the user's PATH entries on `purge`. No `set -e`, +# and a trailing `exit 0`, so a best-effort step never fails the removal. +# +# Unlike after-install.sh and after-remove.sh, this one reaches fpm through the +# raw passthrough in electron-builder.config.ts, so ${macro} is NOT expanded +# here. It finds its own paths at runtime instead. +# +# WHY A COPY. Engine-manager records what it added to PATH under the user's data +# root (engine-bin/engine-path/), while the entries themselves live in the login +# shell's profiles, which no maintainer script touches. Only a purge deletes the +# data root, and only postrm can tell a purge from a plain remove -- prerm sees +# "remove" either way. But dpkg deletes the package's files before postrm runs, +# and a purge can come long after the remove, so the binary that understands the +# records has to outlive the package. It is kept under /var/lib/, which +# after-remove deletes on purge. +# +# Releasing here instead would give up the entries on a plain `apt remove`, which +# keeps the engines: a reinstalled app would then have nothing that republishes +# them short of reinstalling each engine. +# +# dpkg calls prerm with "upgrade" during an update, and "failed-upgrade" when +# recovering from one. Those keep the installation, so there is nothing to keep. +case "${1:-}" in + upgrade|failed-upgrade) + exit 0 + ;; +esac + +# Ask dpkg where it put the binary rather than rebuilding /opt//... +# from a name this script cannot be told at build time. +package="${DPKG_MAINTSCRIPT_PACKAGE:-}" +[ -n "$package" ] || exit 0 +command -v dpkg-query >/dev/null 2>&1 || exit 0 +engine_manager="$(dpkg-query -L "$package" 2>/dev/null \ + | grep -E '/cli-bin/nvpair-engine-manager$' | head -n 1)" +[ -n "$engine_manager" ] && [ -x "$engine_manager" ] || exit 0 + +mkdir -p "/var/lib/$package" 2>/dev/null \ + && install -m 0755 "$engine_manager" "/var/lib/$package/nvpair-engine-manager" 2>/dev/null \ + || true + +exit 0 diff --git a/desktop/scripts/build/macos/uninstall.sh b/desktop/scripts/build/macos/uninstall.sh index ec948cc8..585531a7 100644 --- a/desktop/scripts/build/macos/uninstall.sh +++ b/desktop/scripts/build/macos/uninstall.sh @@ -98,6 +98,45 @@ if [ -x "$CTL" ]; then fi fi +# Release the PATH entries this user's engines own, while the binary that owns +# the ownership records still exists. +# +# Engine-manager records what it added under the data root +# (engine-bin/engine-path/), while the entries themselves live in the login +# shell's profiles — which the purge below never touches. Removing the records +# first would strand those entries with no way left to identify them, pointing +# at engine directories this script is about to delete. Only needed when data is +# going away: keeping it keeps the engines, the records, and a reinstall's +# ability to clean up later. +# +# Runs as the invoking user for the same reason as the helper above: the +# profiles and the records are theirs, not root's. +# +# Unlike every other step here, a failure is not shrugged off. The binary +# removes what it can and reports the rest, and the records that could still +# identify whatever it left are inside the data root the purge is about to +# delete — so a failed release cancels the purge rather than making those +# entries unidentifiable. The app bundle still goes; a reinstall retries. +if [ "$PURGE_DATA" = "1" ]; then + EM="$APP_PATH/Contents/Resources/cli-bin/nvpair-engine-manager" + if [ -x "$EM" ]; then + echo "Releasing engine PATH entries..." + # `|| released=$?` rather than a bare call: set -e is on, so a failure would + # otherwise abort before the check below could keep the data. + released=0 + if [ -n "$real_user" ] && [ "$real_user" != "root" ]; then + sudo -u "$real_user" "$EM" --remove-user-path >/dev/null 2>&1 || released=$? + else + "$EM" --remove-user-path >/dev/null 2>&1 || released=$? + fi + if [ "$released" -ne 0 ]; then + echo "Warning: could not release every engine PATH entry." >&2 + echo "Keeping user data so a reinstall can finish the cleanup; re-run with --purge afterwards." >&2 + PURGE_DATA=0 + fi + fi +fi + echo "Removing $APP_PATH ..." rm -rf "$APP_PATH" 2>/dev/null || true diff --git a/desktop/src/electron/service-bridge/empty-handlers.ts b/desktop/src/electron/service-bridge/empty-handlers.ts index 14fd3d89..21b295a7 100644 --- a/desktop/src/electron/service-bridge/empty-handlers.ts +++ b/desktop/src/electron/service-bridge/empty-handlers.ts @@ -155,6 +155,46 @@ async function toggleLocalEngine(engine: string, engineType: EngineType): Promis } } +/** + * Update is an uninstall followed by an install. The engine-manager serializes + * per-engine ops via its lifecycle lock, so the queued install waits for the + * uninstall to finish; the uninstall's engine:state-changed briefly clears the + * pending status, then the install's progress re-establishes `installing`. + * + * It carries the user's earlier PATH answer through both steps rather than + * asking again. That answer is read fresh, because `nvpair-tui` runs its own + * engine-manager against the same PATH records and its changes never reach this + * bridge as a push. + */ +async function updateLocalEngine( + engine: string, + engineType: EngineType, + failPendingOp: (action: string, operation: 'install' | 'uninstall') => (error: string) => void +): Promise { + const supervisor = getModularSupervisor() + let pathManaged = getModularBridgeState().localEnginePathManaged(engineType) + try { + const status = await supervisor.callProcess('broker', 'engine:status', { engine }) + pathManaged = booleanValue(objectValue(status)?.path_managed) + } catch { + // The last pushed state is the best answer left. + } + supervisor.sendProcess( + 'broker', + 'engine:uninstall', + pathManaged ? { engine, path: true } : { engine }, + failPendingOp('update', 'uninstall'), + true + ) + supervisor.sendProcess( + 'broker', + 'engine:install', + { engine, start: true, path: pathManaged }, + failPendingOp('update', 'install'), + true + ) +} + /** * Dispatch a UI engine command to the local `nvpair-engine-manager`. Lifecycle and * model operations are **fire-and-forget**: the engine-manager runs each in its @@ -203,41 +243,26 @@ function routeEngineManagerCommand(payload: WsInvokeRequest<'engine:command'>): supervisor.sendProcess( 'broker', 'engine:install', - { engine, start: true }, + { engine, start: true, path: payload.path === true }, failPendingOp('install', 'install'), true ) break case 'uninstall': state.beginLocalEngineOp(payload.engineType, 'uninstalling') + // An absent answer is forwarded as absent: the engine-manager then + // keeps its claim on PATH entries instead of handing them over. supervisor.sendProcess( 'broker', 'engine:uninstall', - { engine }, + payload.path === undefined ? { engine } : { engine, path: payload.path }, failPendingOp('uninstall', 'uninstall'), true ) break case 'update': - // The engine-manager serializes per-engine ops via its lifecycle - // lock, so the queued install waits for the uninstall to finish. The - // uninstall's engine:state-changed briefly clears this, then the - // install's progress re-establishes `installing`. state.beginLocalEngineOp(payload.engineType, 'installing') - supervisor.sendProcess( - 'broker', - 'engine:uninstall', - { engine }, - failPendingOp('update', 'uninstall'), - true - ) - supervisor.sendProcess( - 'broker', - 'engine:install', - { engine, start: true }, - failPendingOp('update', 'install'), - true - ) + void updateLocalEngine(engine, payload.engineType, failPendingOp) break case 'toggle': void toggleLocalEngine(engine, payload.engineType) diff --git a/desktop/src/electron/service-bridge/modular-state.ts b/desktop/src/electron/service-bridge/modular-state.ts index 151f1745..8608fbce 100644 --- a/desktop/src/electron/service-bridge/modular-state.ts +++ b/desktop/src/electron/service-bridge/modular-state.ts @@ -903,7 +903,7 @@ class ModularBridgeState { */ private engineManagerFacts = new Map< EngineType, - { installed: boolean; running: boolean; port: number } + { installed: boolean; running: boolean; port: number; pathManaged: boolean } >() /** * Local model lists pulled from `nvpair-engine-manager`'s `list_models` action by @@ -1368,7 +1368,8 @@ class ModularBridgeState { this.engineManagerFacts.set(engineType, { installed: booleanValue(obj.installed), running: booleanValue(obj.running), - port: numberValue(obj.port) + port: numberValue(obj.port), + pathManaged: booleanValue(obj.path_managed) }) // A fresh authoritative state is the resolution of whatever op was in // flight (start/stop done, install `done`+installed, uninstall removed). @@ -1378,6 +1379,11 @@ class ModularBridgeState { this.emitLocalEngineStatus(engineType) } + /** Whether PAIR owns a PATH entry for a local engine, per the engine-manager. */ + localEnginePathManaged(engineType: EngineType): boolean { + return this.engineManagerFacts.get(engineType)?.pathManaged ?? false + } + /** * Begin an optimistic transitional status for a local-engine lifecycle op so * the UI shows a spinner / progress line immediately. The backend never emits @@ -2068,7 +2074,8 @@ class ModularBridgeState { nodeId, processStatus: pending, enginePort: facts && facts.running && facts.port > 0 ? facts.port : null, - proxyPort: isProxyEngine(engineType) ? this.getProxyPort(engineType) : null + proxyPort: isProxyEngine(engineType) ? this.getProxyPort(engineType) : null, + pathManaged: facts?.pathManaged ?? false } } @@ -2091,7 +2098,8 @@ class ModularBridgeState { // Each proxy-fronted engine has its own broker proxy // (`ollama-proxy` / `lmstudio-proxy`); report that engine's bound // proxy port. Loopback-only engines get null. - proxyPort: isProxyEngine(engineType) ? this.getProxyPort(engineType) : null + proxyPort: isProxyEngine(engineType) ? this.getProxyPort(engineType) : null, + pathManaged: facts.pathManaged } } diff --git a/desktop/src/shared/types/engine-api.ts b/desktop/src/shared/types/engine-api.ts index 9187e658..d411200d 100644 --- a/desktop/src/shared/types/engine-api.ts +++ b/desktop/src/shared/types/engine-api.ts @@ -57,6 +57,13 @@ export interface EngineCommandPayload { nodeId: string model?: string expiry?: string + /** + * The user's answer to changing their PATH, for `install` (add the engine's + * command-line tools) and `uninstall` (remove what PAIR added; false keeps + * the entry as the user's). Absent means the user was not asked: PATH is left + * as it is, and an uninstall keeps PAIR's claim on any entry it added. + */ + path?: boolean } /** Canonical engine state snapshot sent on connect/reconnect. */ diff --git a/desktop/src/shared/types/engines.ts b/desktop/src/shared/types/engines.ts index 863c415c..e009bfad 100644 --- a/desktop/src/shared/types/engines.ts +++ b/desktop/src/shared/types/engines.ts @@ -49,6 +49,12 @@ export interface EngineStatusData { * reported version data or engines that are not installed. */ installedVersion?: string + /** + * True while PAIR owns a PATH entry for this engine's command-line tools on + * the local node, which is what decides whether uninstall offers to remove + * one. Only the local node reports it. + */ + pathManaged?: boolean } export type ModelItemStatus = (typeof ModelItemStatuses)[number] diff --git a/desktop/src/ui/api/engine-api.ts b/desktop/src/ui/api/engine-api.ts index 71d8f163..af6af7a3 100644 --- a/desktop/src/ui/api/engine-api.ts +++ b/desktop/src/ui/api/engine-api.ts @@ -48,10 +48,15 @@ export interface IEngineApi { /** Start or stop an engine process on a node. */ toggle(engineType: EngineType, nodeId: string): void - /** Install an engine on a node. */ - install(engineType: EngineType, nodeId: string): void - /** Uninstall an engine from a node without removing downloaded models. */ - uninstall(engineType: EngineType, nodeId: string): void + /** Install an engine on a node, adding its command-line tools to PATH when the user agreed. */ + install(engineType: EngineType, nodeId: string, addToPath: boolean): void + /** + * Uninstall an engine from a node without removing downloaded models. + * `removePath` is the user's answer about the PATH entry PAIR added: true + * removes it, false hands it to the user, and omitted (not asked) keeps + * PAIR's claim on it. + */ + uninstall(engineType: EngineType, nodeId: string, removePath?: boolean): void /** Pull (download) a model on a node. */ pullModel(engineType: EngineType, nodeId: string, model: string): void /** Load a model into memory on a node. */ @@ -94,10 +99,10 @@ export function createEngineApi(transport: ServiceTransport): IEngineApi { toggle: (engineType, nodeId) => fireCommand(transport, { command: 'toggle', engineType, nodeId }), - install: (engineType, nodeId) => - fireCommand(transport, { command: 'install', engineType, nodeId }), - uninstall: (engineType, nodeId) => - fireCommand(transport, { command: 'uninstall', engineType, nodeId }), + install: (engineType, nodeId, addToPath) => + fireCommand(transport, { command: 'install', engineType, nodeId, path: addToPath }), + uninstall: (engineType, nodeId, removePath) => + fireCommand(transport, { command: 'uninstall', engineType, nodeId, path: removePath }), pullModel: (engineType, nodeId, model) => fireCommand(transport, { command: 'pullModel', engineType, nodeId, model }), loadModel: (engineType, nodeId, model) => diff --git a/desktop/src/ui/components/BackendRow/BackendRow.tsx b/desktop/src/ui/components/BackendRow/BackendRow.tsx index f3c16238..efeba440 100644 --- a/desktop/src/ui/components/BackendRow/BackendRow.tsx +++ b/desktop/src/ui/components/BackendRow/BackendRow.tsx @@ -17,6 +17,7 @@ import { usePendingActionsStore } from '@/ui/stores/pending-actions.store' import type { EngineCommandType } from '@/shared/types/engine-api' import { ConfirmModal } from '@/ui/components/ConfirmModal' +import { EnginePathConsentModal } from '@/ui/components/EnginePathConsentModal' import { ModelSection } from '@/ui/components/ModelManager/ModelSection' import { BackendHeader } from './BackendHeader' import { BackendFooter } from './BackendFooter' @@ -62,6 +63,11 @@ export function BackendRow({ }) { const [expanded, setExpanded] = useState(false) const [confirmUninstall, setConfirmUninstall] = useState(false) + const [askPath, setAskPath] = useState(false) + const [removePath, setRemovePath] = useState(true) + const pathManaged = useEngineStatusStore( + s => s.statusByNode.get(nodeId)?.get(backend.type)?.pathManaged === true + ) const installProgress = useEngineProgressStore(s => { const installKey = engineProgressKey({ nodeId, @@ -151,19 +157,30 @@ export function BackendRow({ window.pairApi.engines.toggle(backend.type, nodeId) }, [backend.type, nodeId]) + // PATH is a change to this machine's user, so only a local install asks. const handleInstall = useCallback(() => { - window.pairApi.engines.install(backend.type, nodeId) - }, [backend.type, nodeId]) + if (isLocalNode) setAskPath(true) + else window.pairApi.engines.install(backend.type, nodeId, false) + }, [backend.type, nodeId, isLocalNode]) + + const handlePathAnswer = useCallback( + (addToPath: boolean) => { + setAskPath(false) + window.pairApi.engines.install(backend.type, nodeId, addToPath) + }, + [backend.type, nodeId] + ) const handleUninstall = useCallback(() => { - window.pairApi.engines.uninstall(backend.type, nodeId) - }, [backend.type, nodeId]) + window.pairApi.engines.uninstall(backend.type, nodeId, pathManaged ? removePath : undefined) + }, [backend.type, nodeId, pathManaged, removePath]) const handleUpdate = useCallback(() => { window.pairApi.engines.update(backend.type, nodeId) }, [backend.type, nodeId]) const requestUninstall = useCallback(() => { + setRemovePath(true) setConfirmUninstall(true) }, []) @@ -235,8 +252,24 @@ export function BackendRow({ message={`Are you sure you want to uninstall ${backend.displayName}?`} confirmLabel="Uninstall" confirmColor="danger" + option={ + pathManaged + ? { + label: `Also remove ${backend.displayName} from my PATH`, + checked: removePath, + onCheckedChange: setRemovePath + } + : undefined + } onConfirm={handleUninstall} /> + + setAskPath(false)} + /> ) } diff --git a/desktop/src/ui/components/ConfirmModal.tsx b/desktop/src/ui/components/ConfirmModal.tsx index a67c6604..edbcf223 100644 --- a/desktop/src/ui/components/ConfirmModal.tsx +++ b/desktop/src/ui/components/ConfirmModal.tsx @@ -4,6 +4,7 @@ import type { ReactNode } from 'react' import { Button, + Checkbox, Flex, ModalContent, ModalDialog, @@ -13,6 +14,13 @@ import { Text } from '@nvidia/foundations-react-core' +/** A controlled checkbox the confirmed action reads when it runs. */ +interface ConfirmModalOption { + label: string + checked: boolean + onCheckedChange: (checked: boolean) => void +} + export function ConfirmModal({ open, onOpenChange, @@ -20,6 +28,7 @@ export function ConfirmModal({ message, confirmLabel, confirmColor = 'brand', + option, onConfirm }: { open: boolean @@ -28,6 +37,7 @@ export function ConfirmModal({ message: ReactNode confirmLabel: string confirmColor?: 'brand' | 'danger' + option?: ConfirmModalOption onConfirm: () => void }) { const handleClose = () => onOpenChange(false) @@ -41,6 +51,15 @@ export function ConfirmModal({
{message}
 + {option && ( + + option.onCheckedChange(checked === true) + } + slotLabel={option.label} + /> + )} + + + + + + + ) +} diff --git a/desktop/src/ui/components/ErrorModal.tsx b/desktop/src/ui/components/ErrorModal.tsx index dde1d82b..4d7f4d9b 100644 --- a/desktop/src/ui/components/ErrorModal.tsx +++ b/desktop/src/ui/components/ErrorModal.tsx @@ -8,9 +8,15 @@ import { errorDismissalKey } from '@/ui/utils/error-modal-dismissal' import { InlineErrorBanner } from './InlineErrorBanner' import { useErrorsStore } from '@/ui/stores/errors.store' import { useNodesStore } from '@/ui/stores/nodes.store' +import { useConnectionStore } from '@/ui/stores/connection.store' +import { useEngineStatusStore } from '@/ui/stores/engine-status.store' import { isEngineType } from '@/shared/utils/engines' +import { EngineDisplayNames } from '@/shared/constants/engines' +import type { EngineType } from '@/shared/types/engines' import type { ServiceError } from '@/shared/types/errors' import type { NodeItem } from '@/shared/types/nodes' +import { ConfirmModal } from './ConfirmModal' +import { EnginePathConsentModal } from './EnginePathConsentModal' interface ErrorModalFilter { nodeId?: string @@ -94,25 +100,33 @@ function bannerStatus(severity: ServiceError['severity']): 'info' | 'warning' | } /** - * Build the retry handler for a `retry` action error by re-running the failed - * engine operation from the fields the backend stamped on the error. Returns - * `null` (no button) when the action isn't `retry` or the error lacks the - * engine/node/model context needed to re-dispatch. + * How a `retry` action error re-runs the failed engine operation from the + * fields the backend stamped on it. Install and uninstall change PATH, so they + * carry the target and are asked about before they run. */ -function buildRetry(error: ServiceError): (() => void) | null { +type RetryAction = + | { kind: 'install' | 'uninstall'; engineType: EngineType; nodeId: string } + | { kind: 'run'; run: () => void } + +/** + * Returns `null` (no button) when the action isn't `retry` or the error lacks + * the engine/node/model context needed to re-dispatch. + */ +function buildRetry(error: ServiceError): RetryAction | null { if (error.action !== 'retry') return null const { engineType, nodeId, operation, modelName } = error if (!engineType || !isEngineType(engineType) || !nodeId) return null const engines = window.pairApi.engines switch (operation) { case 'install': - return () => engines.install(engineType, nodeId) case 'uninstall': - return () => engines.uninstall(engineType, nodeId) + return { kind: operation, engineType, nodeId } case 'start': - return () => engines.toggle(engineType, nodeId) + return { kind: 'run', run: () => engines.toggle(engineType, nodeId) } case 'pull': - return modelName ? () => engines.pullModel(engineType, nodeId, modelName) : null + return modelName + ? { kind: 'run', run: () => engines.pullModel(engineType, nodeId, modelName) } + : null default: return null } @@ -164,21 +178,94 @@ function ErrorBanner({ const displayMessage = formatErrorMessage(error, nodeLabel, showNodePrefix) const severity = bannerStatus(error.severity) const retry = buildRetry(error) + const selfId = useConnectionStore(state => state.selfId) + const retryTarget = retry && retry.kind !== 'run' ? retry : null + const pathManaged = useEngineStatusStore(state => + retryTarget + ? state.statusByNode.get(retryTarget.nodeId)?.get(retryTarget.engineType) + ?.pathManaged === true + : false + ) + const [askInstallPath, setAskInstallPath] = useState(false) + const [askUninstallPath, setAskUninstallPath] = useState(false) + const [removePath, setRemovePath] = useState(false) + + // Clearing the error unmounts this banner, so it waits for any PATH answer. + const rerun = (run: () => void) => { + onClear(error.id) + run() + } + + const handleRetry = () => { + if (!retry) return + if (retry.kind === 'run') { + rerun(retry.run) + return + } + const { engineType, nodeId } = retry + const isLocal = nodeId === selfId + if (retry.kind === 'install') { + if (isLocal) setAskInstallPath(true) + else rerun(() => window.pairApi.engines.install(engineType, nodeId, false)) + return + } + if (pathManaged) { + // Unchecked: the failed attempt may have been one that kept the entry. + setRemovePath(false) + setAskUninstallPath(true) + } else { + rerun(() => window.pairApi.engines.uninstall(engineType, nodeId)) + } + } return ( {retry && ( - )} + {retryTarget && ( + <> + { + setAskInstallPath(false) + rerun(() => + window.pairApi.engines.install( + retryTarget.engineType, + retryTarget.nodeId, + addToPath + ) + ) + }} + onCancel={() => setAskInstallPath(false)} + /> + + rerun(() => + window.pairApi.engines.uninstall( + retryTarget.engineType, + retryTarget.nodeId, + removePath + ) + ) + } + /> + + )} ) } diff --git a/desktop/src/ui/components/NodeList/NodeEnginesInline.tsx b/desktop/src/ui/components/NodeList/NodeEnginesInline.tsx index 8dfafe4f..ce01fe30 100644 --- a/desktop/src/ui/components/NodeList/NodeEnginesInline.tsx +++ b/desktop/src/ui/components/NodeList/NodeEnginesInline.tsx @@ -10,7 +10,8 @@ import { usePendingActionsStore } from '@/ui/stores/pending-actions.store' import { getEnginesForNode } from '@/ui/utils/get-engines-for-node' import { Button, Flex, Switch, Text } from '@nvidia/foundations-react-core' import { Download } from '@/ui/components/icons' -import { useCallback, useMemo } from 'react' +import { EnginePathConsentModal } from '@/ui/components/EnginePathConsentModal' +import { useCallback, useMemo, useState } from 'react' export default function NodeEnginesInline({ nodeId }: { nodeId: string }) { const statusByNode = useEngineStatusStore(s => s.statusByNode) @@ -64,15 +65,33 @@ export default function NodeEnginesInline({ nodeId }: { nodeId: string }) { [nodeId] ) + const [askPathFor, setAskPathFor] = useState(null) + + // PATH is a change to this machine's user, so only a local install asks. const handleInstall = useCallback( (engineType: EngineType) => { - window.pairApi.engines.install(engineType, nodeId) + if (isRemote) window.pairApi.engines.install(engineType, nodeId, false) + else setAskPathFor(engineType) }, - [nodeId] + [isRemote, nodeId] + ) + + const handlePathAnswer = useCallback( + (addToPath: boolean) => { + if (askPathFor) window.pairApi.engines.install(askPathFor, nodeId, addToPath) + setAskPathFor(null) + }, + [askPathFor, nodeId] ) return ( <> + setAskPathFor(null)} + /> {allBackends.map(b => { const pending = Boolean( usePendingActionsStore.getState().getLifecyclePending(nodeId, b.type) diff --git a/desktop/src/ui/components/Welcome/WelcomeModal.tsx b/desktop/src/ui/components/Welcome/WelcomeModal.tsx index 101da247..37800de9 100644 --- a/desktop/src/ui/components/Welcome/WelcomeModal.tsx +++ b/desktop/src/ui/components/Welcome/WelcomeModal.tsx @@ -4,6 +4,7 @@ import { useCallback, useEffect, useMemo, useRef, useState } from 'react' import { ModalContent, ModalDialog, ModalRoot, Stack, Text } from '@nvidia/foundations-react-core' import { DialogHeader } from '@/ui/components/DialogHeader' +import { EnginePathConsentModal } from '@/ui/components/EnginePathConsentModal' import { InlineErrorBanner } from '@/ui/components/InlineErrorBanner' import { useBlurOnOpen } from '@/ui/hooks/useBlurOnOpen' import { useEngineStatusStore } from '@/ui/stores/engine-status.store' @@ -46,6 +47,7 @@ export function WelcomeModal({ open, onOpenChange, selfId }: WelcomeModalProps) > | null>(null) const [installing, setInstalling] = useState(false) const [installTargets, setInstallTargets] = useState([]) + const [pathPromptTargets, setPathPromptTargets] = useState([]) const [error, setError] = useState(null) const startedTargets = useRef(new Set()) const failedTargets = useRef(new Set()) @@ -64,6 +66,7 @@ export function WelcomeModal({ open, onOpenChange, selfId }: WelcomeModalProps) setError(null) setInstalling(false) setInstallTargets([]) + setPathPromptTargets([]) startedTargets.current.clear() failedTargets.current.clear() } @@ -140,13 +143,23 @@ export function WelcomeModal({ open, onOpenChange, selfId }: WelcomeModalProps) finishWelcome(true) return } - setInstalling(true) - startedTargets.current.clear() - failedTargets.current.clear() - knownErrors.current = new Map(errors.map(item => [item.id, item.timestamp])) - setInstallTargets(targets) - targets.forEach(t => window.pairApi.engines.install(t, selfId)) - }, [candidates, engineSelections, selfId, isEngineInstallable, finishWelcome, errors]) + setPathPromptTargets(targets) + }, [candidates, engineSelections, isEngineInstallable, finishWelcome]) + + /** One PATH answer covers every engine the wizard installs. */ + const handlePathAnswer = useCallback( + (addToPath: boolean) => { + const targets = pathPromptTargets + setPathPromptTargets([]) + setInstalling(true) + startedTargets.current.clear() + failedTargets.current.clear() + knownErrors.current = new Map(errors.map(item => [item.id, item.timestamp])) + setInstallTargets(targets) + targets.forEach(t => window.pairApi.engines.install(t, selfId, addToPath)) + }, + [errors, pathPromptTargets, selfId] + ) useEffect(() => { if (!installing || installTargets.length === 0) return @@ -236,6 +249,12 @@ export function WelcomeModal({ open, onOpenChange, selfId }: WelcomeModalProps) onInstall={handleInstall} /> + 0} + engines={pathPromptTargets} + onAnswer={handlePathAnswer} + onCancel={() => setPathPromptTargets([])} + /> diff --git a/desktop/tests/modular/engine-path-consent.test.ts b/desktop/tests/modular/engine-path-consent.test.ts new file mode 100644 index 00000000..aefddb8d --- /dev/null +++ b/desktop/tests/modular/engine-path-consent.test.ts @@ -0,0 +1,121 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const mocks = vi.hoisted(() => ({ + state: { + getSelfId: vi.fn(() => 'local-node'), + beginLocalEngineOp: vi.fn(), + clearPendingEngineOp: vi.fn(), + localEnginePathManaged: vi.fn(() => false) + }, + supervisor: { + hasProcess: vi.fn(() => true), + callProcess: vi.fn(), + sendProcess: vi.fn(), + reportError: vi.fn() + } +})) + +vi.mock('@/electron/service-bridge/modular-supervisor', () => ({ + getModularSupervisor: () => mocks.supervisor +})) +vi.mock('@/electron/service-bridge/modular-state', () => ({ + getModularBridgeState: () => mocks.state, + isProxyEngine: () => false, + isUpstreamUnreachableError: () => false, + parseServiceErrors: () => [] +})) +vi.mock('@/electron/model-hub', () => ({ getEngineHubModels: vi.fn() })) + +import { handleServiceBridgeInvoke } from '@/electron/service-bridge/empty-handlers' +import type { JsonValue } from '@/shared/types/json' + +function sentParams(method: string): JsonValue[] { + return mocks.supervisor.sendProcess.mock.calls + .filter(call => call[1] === method) + .map(call => call[2]) +} + +describe('engine PATH consent on local lifecycle commands', () => { + beforeEach(() => { + mocks.supervisor.sendProcess.mockClear() + mocks.state.localEnginePathManaged.mockReturnValue(false) + }) + + it('forwards the install answer to engine:install', async () => { + await handleServiceBridgeInvoke('engine:command', { + command: 'install', + engineType: 'ollama', + nodeId: 'local-node', + path: true + }) + await handleServiceBridgeInvoke('engine:command', { + command: 'install', + engineType: 'ollama', + nodeId: 'local-node', + path: false + }) + + expect(sentParams('engine:install')).toEqual([ + { engine: 'ollama', start: true, path: true }, + { engine: 'ollama', start: true, path: false } + ]) + }) + + it('forwards the uninstall answer, and sends none when the user was not asked', async () => { + for (const path of [true, false, undefined]) { + await handleServiceBridgeInvoke('engine:command', { + command: 'uninstall', + engineType: 'ollama', + nodeId: 'local-node', + path + }) + } + + expect(sentParams('engine:uninstall')).toEqual([ + { engine: 'ollama', path: true }, + { engine: 'ollama', path: false }, + { engine: 'ollama' } + ]) + }) + + it('carries the fresh PATH state through both steps of an update', async () => { + mocks.supervisor.callProcess.mockResolvedValueOnce({ engine: 'ollama', path_managed: true }) + + await handleServiceBridgeInvoke('engine:command', { + command: 'update', + engineType: 'ollama', + nodeId: 'local-node' + }) + + await vi.waitFor(() => { + expect(sentParams('engine:uninstall')).toEqual([{ engine: 'ollama', path: true }]) + expect(sentParams('engine:install')).toEqual([ + { engine: 'ollama', start: true, path: true } + ]) + }) + }) + + it('updates an engine without a PATH entry without touching PATH', async () => { + mocks.state.localEnginePathManaged.mockReturnValue(true) + mocks.supervisor.callProcess.mockResolvedValueOnce({ + engine: 'ollama', + path_managed: false + }) + + await handleServiceBridgeInvoke('engine:command', { + command: 'update', + engineType: 'ollama', + nodeId: 'local-node' + }) + + await vi.waitFor(() => { + expect(sentParams('engine:uninstall')).toEqual([{ engine: 'ollama' }]) + expect(sentParams('engine:install')).toEqual([ + { engine: 'ollama', start: true, path: false } + ]) + }) + }) +}) diff --git a/desktop/tests/modular/engine-path-managed-state.test.ts b/desktop/tests/modular/engine-path-managed-state.test.ts new file mode 100644 index 00000000..5105317a --- /dev/null +++ b/desktop/tests/modular/engine-path-managed-state.test.ts @@ -0,0 +1,54 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +import { afterEach, describe, expect, it, vi } from 'vitest' + +vi.mock('electron', () => ({ BrowserWindow: { getAllWindows: () => [] } })) +vi.mock('@/electron/window', () => ({ createOverviewWindow: vi.fn() })) + +import { subscribePush } from '@/electron/service-bridge/push-bus' +import { getModularBridgeState } from '@/electron/service-bridge/modular-state' + +let unsubscribe = (): void => {} + +afterEach(() => { + unsubscribe() + unsubscribe = () => {} +}) + +describe('local engine PATH ownership', () => { + it('projects the engine-manager path_managed fact onto the local status', () => { + const state = getModularBridgeState() + const reported: (boolean | undefined)[] = [] + unsubscribe = subscribePush(event => { + if ( + event.channel === 'engines:state-changed' && + event.payload.engineType === 'ollama' && + event.payload.status + ) { + reported.push(event.payload.status.pathManaged) + } + }) + state.setSelfId('local-node') + + state.applyEngineManagerStatus({ + engine: 'ollama', + installed: true, + running: false, + port: 11434, + path_managed: true + }) + expect(reported.at(-1)).toBe(true) + expect(state.localEnginePathManaged('ollama')).toBe(true) + + state.applyEngineManagerStatus({ + engine: 'ollama', + installed: false, + running: false, + port: 11434, + path_managed: false + }) + expect(reported.at(-1)).toBe(false) + expect(state.localEnginePathManaged('ollama')).toBe(false) + }) +}) diff --git a/docs/engine-lifecycle.mdx b/docs/engine-lifecycle.mdx index 0d492277..a9c2ad46 100644 --- a/docs/engine-lifecycle.mdx +++ b/docs/engine-lifecycle.mdx @@ -70,6 +70,30 @@ Refer also to [Getting started](getting-started.mdx). You can install a supported engine during first-run setup or later from **Engine settings**. +Before an install, PAIR asks whether to add the engine's command-line tools +(`ollama` or `lms`) to your PATH. First-run setup asks once for every engine you +selected. Answering **No** still installs the engine; closing the question +cancels the install. The terminal interface asks the same question. If you +answer **Yes**, open a new terminal after installation; on Windows, you may need +to restart your terminal app. + +PATH setup only ever changes the account that asked for the install. Installing +an engine on another node from this one leaves that node's PATH alone, and an +engine that was already on the machine before PAIR keeps whatever its own +installer set up. + +If PATH setup fails, PAIR reports it as a dismissible warning. The install still +succeeds, the engine still starts, and it works normally through its full path. + +If PAIR added an engine to your PATH, the uninstall confirmation includes +**Also remove it from my PATH**, checked by default. PAIR removes only the PATH +entries it recorded adding. Pre-existing entries and user-edited shell blocks +are preserved. Installations without an ownership record are left unchanged. If +you clear the box, the entry stays and is yours from then on: PAIR no longer +tracks it. Uninstalling PAIR itself releases any entries its engines still own +when it also removes PAIR's data; on Linux that is `apt purge`, while a plain +`apt remove` keeps both the engines and their PATH entries. + When you install an engine, consider the following: - Choosing install makes it an **NVPAIR-installed** engine. PAIR downloads it, diff --git a/docs/getting-started.mdx b/docs/getting-started.mdx index cb7e7e34..2fe81a3f 100644 --- a/docs/getting-started.mdx +++ b/docs/getting-started.mdx @@ -351,51 +351,60 @@ current URL. ### Using an Engine's Own Command Line An engine PAIR installed for you is a normal installation, and you can drive it -with its own command-line interface (CLI). Two things make that less obvious: +with its own command-line interface (CLI). -- The binaries are not on your `PATH` yet. -- The engine is not on the port its CLI expects by default. +When PAIR installs an engine, it asks whether to add that engine's command +directory to your user `PATH`. If you agree, `ollama` or `lms` works by name. +**Open a new terminal afterwards** — a shell that was already running keeps the +`PATH` it started with. When you uninstall the engine, PAIR offers to remove the +entry again. If you declined, or PAIR could not update your `PATH`, the engine +still works; use the full path below. + +One thing still catches people out: the engine is not on the port its CLI expects +by default. Refer to [Set the port explicitly](#set-the-port-explicitly) below. **Ollama** is installed inside PAIR's own data directory: -| Platform | Path | -| --- | --- | -| Windows | `%LOCALAPPDATA%\Nvidia Corporation\Personal AI Router\engine-bin\ollama\ollama.exe` | -| Linux | `~/.config/Nvidia Corporation/Personal AI Router/engine-bin/ollama/bin/ollama` | +```powershell +$env:OLLAMA_HOST = "127.0.0.1:11435" +ollama list +``` -On Linux it needs its bundled libraries on the library path: +On Linux, Ollama also needs its bundled libraries on the library path: ```bash ENGINE="$HOME/.config/Nvidia Corporation/Personal AI Router/engine-bin/ollama" -LD_LIBRARY_PATH="$ENGINE/lib/ollama" OLLAMA_HOST=127.0.0.1:11435 "$ENGINE/bin/ollama" list -``` - -```powershell -$ollama = "$env:LOCALAPPDATA\Nvidia Corporation\Personal AI Router\engine-bin\ollama\ollama.exe" -$env:OLLAMA_HOST = "127.0.0.1:11435" -& $ollama list +LD_LIBRARY_PATH="$ENGINE/lib/ollama" OLLAMA_HOST=127.0.0.1:11435 ollama list ``` **LM Studio** installs to its own standard location instead, because PAIR runs its -official installer: `~/.lmstudio/bin/lms`, or -`%USERPROFILE%\.lmstudio\bin\lms.exe` on Windows. +official installer: ```bash -~/.lmstudio/bin/lms status +lms status ``` -**Set the port explicitly.** Set `OLLAMA_HOST` to the engine's own port. -Otherwise, the CLI connects to `11434`, which is PAIR's proxy, and `ollama list` -returns the cluster's view instead of the local machine's. Use the **Server** -value under **Engine settings > Ports** to address the local engine directly. -It defaults to `11435` for Ollama and `1235` for LM Studio. +If you need the full path — because you declined `PATH` setup, it failed, or you are scripting +against a specific installation — these are the locations: + +| Engine | Platform | Path | +| --- | --- | --- | +| Ollama | Windows | `%LOCALAPPDATA%\Nvidia Corporation\Personal AI Router\engine-bin\ollama\ollama.exe` | +| Ollama | Linux | `~/.config/Nvidia Corporation/Personal AI Router/engine-bin/ollama/bin/ollama` | +| LM Studio | Windows | `%USERPROFILE%\.lmstudio\bin\lms.exe` | +| LM Studio | Linux, macOS | `~/.lmstudio/bin/lms` | + +#### Set the Port Explicitly + +Set `OLLAMA_HOST` to the engine's own port. Otherwise, the CLI connects to +`11434`, which is PAIR's proxy, and `ollama list` returns the cluster's view +instead of the local machine's. Use the **Server** value under +**Engine settings > Ports** to address the local engine directly. It defaults to +`11435` for Ollama and `1235` for LM Studio. Use the proxy port to check what your cluster can serve. Use the engine port to check what is installed on the local machine. -PAIR does not put these binaries on `PATH` yet. Until then, use the full path or -add an alias yourself. - ## Use PAIR with Existing Applications Any client that lets you set a base URL and a model name can use PAIR — Hermes, diff --git a/docs/known-issues.mdx b/docs/known-issues.mdx index 59aa0cdf..52827467 100644 --- a/docs/known-issues.mdx +++ b/docs/known-issues.mdx @@ -81,13 +81,6 @@ This matters most for a genuinely headless deployment, where those operations ha no other route on that machine. Refer to [What the Terminal Interface Cannot Do](terminal-interface.mdx#what-the-terminal-interface-cannot-do). -## Engine Binaries Are Not on Your PATH - -PAIR does not add the engines it installs to your `PATH`, so running an engine's -CLI means using the full path. Refer to -[Using an engine's own command line](getting-started.mdx#using-an-engines-own-command-line) -for the locations. This is on the roadmap. - ## Platform Limits Worth Knowing - **The Linux desktop installer is a `.deb` only.** On RPM-based distributions, diff --git a/scripts/wipe-app-data.ps1 b/scripts/wipe-app-data.ps1 index 4f2caeb7..0a958745 100644 --- a/scripts/wipe-app-data.ps1 +++ b/scripts/wipe-app-data.ps1 @@ -122,6 +122,16 @@ $Tmp = $env:TEMP if (-not $Tmp) { $Tmp = $env:TMP } $ControlDir = Join-Path $Tmp "nvpair-$Scope" +# The engine PATH entry in HKCU\Environment is deliberately NOT listed. Wiping +# the data root deletes engine-manager's ownership records along with the +# engines they describe, so nothing here could identify the entry to remove; the +# real uninstaller drains them first instead (desktop/scripts/build/installer.nsh). +# Recovery on the next install is partial, so prefer the real uninstaller. An +# engine PAIR installed into its own directory is recognized by the executable's +# location and reclaims the entry it finds already present. One whose vendor +# owns its location has no such evidence once the record is gone, so its entry +# is left orphaned until the user removes it. +# # Append-only target list. Never remove entries — only append. $Targets = @( @{ Path = $CurrentRoot; Reason = 'Current shared Electron + backend app data root' }, diff --git a/scripts/wipe-app-data.sh b/scripts/wipe-app-data.sh index 659393d2..b042268e 100755 --- a/scripts/wipe-app-data.sh +++ b/scripts/wipe-app-data.sh @@ -125,6 +125,16 @@ RUNTIME_BASE="${XDG_RUNTIME_DIR:-${TMPDIR:-/tmp}}" RUNTIME_BASE="${RUNTIME_BASE%/}" CONTROL_DIR="$RUNTIME_BASE/nvpair-$SCOPE" +# The engine PATH blocks in the user's shell profiles are deliberately NOT +# listed. Wiping the data root deletes engine-manager's ownership records along +# with the engines they describe, so nothing here could identify the blocks to +# remove; the real uninstallers drain them first instead (desktop/scripts/build). +# Recovery on the next install is partial, so prefer the real uninstallers. An +# engine PAIR installed into its own directory is recognized by the executable's +# location and re-adopts the block it finds already present. One whose vendor +# owns its location — LM Studio writes ~/.lmstudio — has no such evidence once +# the record is gone, so its block is left orphaned until the user removes it. +# # Append-only target list (path|reason). Never remove entries — only append. TARGETS=() TARGETS+=("$CURRENT_ROOT|Current shared Electron + backend app data root") diff --git a/services/nvpair-engine-manager/MANIFEST.md b/services/nvpair-engine-manager/MANIFEST.md index a5f13515..12b77c99 100644 --- a/services/nvpair-engine-manager/MANIFEST.md +++ b/services/nvpair-engine-manager/MANIFEST.md @@ -144,6 +144,21 @@ and recovery. Editing `args`/`start` directly remains trusted manifest authoring | `run` | string[] | no | Argv to execute after download (e.g. run the installer, extract the archive). Placeholders resolved; OS env refs expanded. Requires a `fetch` (the artifact it unpacks). | | `script` | string[] | no | **Escape hatch** for vendors that only ship a script installer. Runs **without** checksum verification (logged as unpinned) and replaces `fetch`+`run`. Prefer `fetch`+`run` whenever the vendor publishes a script or artifact: download it first, then execute the local file. **Make failures loud:** a piped bootstrap such as `curl … \| bash` can mask a failed fetch, while a separate fetch prevents the run and reports the error. | | `mode` | string | no | `"user"` (default) or `"admin"`. The runner **refuses** `"admin"` (engine-manager is user-mode only); it is a deliberate, flagged exception, not a default. | +| `env` | object | no | Literal `KEY: "value"` overrides layered onto the inherited environment of the **installer subprocess only** — not this service's environment and not the engine's runtime (that is `runtime.env`). Values are used verbatim: no placeholders, no OS env expansion. | + +`install.env` exists so a vendor quirk stays in the manifest. LM Studio's +installer edits the user's PATH unless `LMS_NO_MODIFY_PATH=1` is set, and PAIR +publishes that directory itself with an ownership record it can later remove — +two writers, one removable. Declaring it keeps the runner engine-agnostic, so a +third-party engine with the same quirk needs no Go change: + +```json +"install": { + "env": { "LMS_NO_MODIFY_PATH": "1" }, + "fetch": { "url": "https://lmstudio.ai/install.sh" }, + "run": ["bash", "{download}"] +} +``` ### Runtime diff --git a/services/nvpair-engine-manager/README.md b/services/nvpair-engine-manager/README.md index b401c883..0ef07e49 100644 --- a/services/nvpair-engine-manager/README.md +++ b/services/nvpair-engine-manager/README.md @@ -33,8 +33,8 @@ Requests (caller → service): | `engine:get-installed` | — | `{ engines: [EngineStatus] }` | | `engine:describe` | `{ engine }` | the engine's manifest | | `engine:status` | `{ engine }` | `EngineStatus` | -| `engine:install` | `{ engine, start?, port?, bind? }` | `EngineStatus` (after install; also starts it if `start:true`) | -| `engine:uninstall` | `{ engine }` | `EngineStatus` (after removal) | +| `engine:install` | `{ engine, start?, port?, bind?, path? }` | `EngineStatus` (after install; also starts it if `start:true`; publishes the CLI on PATH only if `path:true`) | +| `engine:uninstall` | `{ engine, path? }` | `EngineStatus` (after removal; removes PAIR's PATH entries only if `path:true`) | | `engine:start` | `{ engine, port?, bind? }` | `EngineStatus` (after readiness) | | `engine:stop` | `{ engine }` | `EngineStatus` | | `engine:restart` | `{ engine }` | `EngineStatus` | @@ -51,7 +51,55 @@ Requests (caller → service): | `shutdown` | — | `null` | | `log/set-level` | `{ level }` | `{ level }` | -`EngineStatus` = `{ engine, display_name, installed, running, healthy, port }`. +`EngineStatus` = `{ engine, display_name, installed, running, healthy, port, path_managed }`. +`path_managed` is true while PAIR owns a PATH entry it wrote for the engine on +this machine, so a client asks about removing one only when there is one. It is +emitted after every PATH step, and always false in what the control surface +serves to peers. + +PATH changes need the user's consent, carried as `path` on each request. A +missing or false `path` never touches PATH, so a client that does not ask the +user changes nothing. + +A locally-initiated install with `path:true` publishes the engine's CLI +directory on this user's PATH. Without it the install still records that PAIR +ran the installer, so a later consented install can re-adopt an engine whose +vendor owns its location; an install over an engine already on disk records +nothing, and a claim already on record is kept. Windows updates `HKCU\Environment\Path`; Unix appends a block to the login +shell's profiles, read from the passwd database rather than the inherited +environment. Existing entries are preserved and repeated requests do not +duplicate PAIR's entries. New terminals pick up the change. + +`InstallForPeer` — the path a cluster peer's remote install takes — never +publishes; like a declined install, it records only that PAIR ran the installer. +A PATH failure is a dismissible warning, never an install error, +so the engine stays installed and the caller's optional start step still runs. + +The directory comes from the manifest's `runtime.cli`. An engine that declares +none falls back to its detected executable, and only from inside the directory +PAIR installed into: `detect` deliberately matches vendor layouts PAIR does not +own. An installation PAIR did not place gets no PATH entry and no warning. + +Ownership is recorded under `engine-bin/engine-path/.json` in the user +data directory, written before PATH is touched and guarded by a lock file in the +same directory so a concurrent engine-manager — `nvpair-tui` starts its own — +cannot clobber it. After a successful uninstall with `path:true` PAIR removes +only recorded entries or unchanged shell snippets, then deletes the receipt; +pre-existing entries and unrecorded installations are preserved. Cleanup +failures keep the receipt for a later uninstall retry, even after the executable +is gone. An uninstall with `path:false` leaves the entries in place and deletes +the receipt, so they become the user's and `--remove-user-path` no longer +touches them. An uninstall with no `path` — a client that did not ask — leaves +the entries and keeps PAIR's claim on them, dropping only the installed flag. + +Those receipts live inside the tree the application uninstaller deletes, while +the PATH entries do not, so `--remove-user-path` drains every one of them and +exits. The uninstaller runs it before removing the data directory, and only +when it is removing that directory: on Debian that is `purge`, from a copy of +this binary the package's pre-remove script keeps under `/var/lib/`. + +The LM Studio installer runs with `LMS_NO_MODIFY_PATH=1`, declared as +`install.env` in its manifest, so every PATH change is one PAIR can remove. Notifications (service → caller): `engine:ready{version}`, `engine:state-changed{EngineStatus}`, @@ -238,6 +286,8 @@ its own. | `--reserved-port ` | `0` (off) | Refuse local or remote engine starts and persisted port changes on a parent-owned proxy alias; the broker configures this from `OLLAMA_HOST` | | `--cluster-dir ` | _(none)_ | Cluster identity/pin directory; gates the `ec` surface on and supplies the leaf/pins used to serve it and to dial peers | | `--loaded-poll-interval ` | `5` | Seconds between loaded-model polls that drive `engine:models-changed`; `0` disables the watcher | +| `--user-path` | `true` | Publish an installed engine's CLI directory on the current user's PATH. `--user-path=false` leaves `HKCU\Environment` and the shell profiles alone; used by live tests so a run cannot edit the developer's own account | +| `--remove-user-path` | `false` | Release every PATH entry this user's engines own, then exit. Not a debug affordance: the Windows, macOS, and Debian uninstallers all invoke it before deleting the data directory that holds the ownership records, and gate that deletion on it succeeding | | `--log-level ` | _(env `NVPAIR_LOG_LEVEL` or `info`)_ | `debug` \| `info` \| `warn` \| `error` | | `--version` | | Print version and exit | @@ -285,9 +335,20 @@ with a loud warning. ## Cross-platform One binary compiles and runs on Windows, Linux, and macOS × amd64/arm64. -Per-OS variance lives in the manifest first; OS primitives (process -termination, console hiding) are the only build-tagged Go -(`proc_windows.go` / `proc_unix.go`). +Per-OS variance lives in the manifest first; the build-tagged Go is limited to +OS primitives: + +| Pair | Primitive | +|---|---| +| `proc_windows.go` / `proc_unix.go` | Process termination, console hiding | +| `userpath_windows.go` / `userpath_unix.go` | User PATH persistence — the registry vs. the login shell's profiles | +| `pathlock_windows.go` / `pathlock_unix.go` | Cross-process file locking | +| `syncdir_windows.go` / `syncdir_unix.go` | Flushing a directory entry after a rename | + +`userpath.go` states the two-function contract those PATH pairs implement. +`userpath_shell.go` and `userpath_windows_entries.go` are deliberately +**untagged** so the profile-editing and PATH-parsing rules stay testable on +every platform, not only on the one CI runs. ## Shutdown diff --git a/services/nvpair-engine-manager/controlserver.go b/services/nvpair-engine-manager/controlserver.go index 8a23a5f5..4ea117a7 100644 --- a/services/nvpair-engine-manager/controlserver.go +++ b/services/nvpair-engine-manager/controlserver.go @@ -79,8 +79,14 @@ func (s *controlServer) handleEngines(w http.ResponseWriter, r *http.Request) { http.Error(w, "method not allowed", http.StatusMethodNotAllowed) return } + engines := s.exec.GetInstalled() + // Whether this user's PATH carries an entry is local account configuration, + // and nothing a peer can do acts on it. + for i := range engines { + engines[i].PathManaged = false + } w.Header().Set("Content-Type", "application/json") - _ = json.NewEncoder(w).Encode(map[string]any{"engines": s.exec.GetInstalled()}) + _ = json.NewEncoder(w).Encode(map[string]any{"engines": engines}) } // serveControl runs the ec mTLS control surface on 0.0.0.0:port until ctx is diff --git a/services/nvpair-engine-manager/controlstream.go b/services/nvpair-engine-manager/controlstream.go index 13bd584d..86104ff9 100644 --- a/services/nvpair-engine-manager/controlstream.go +++ b/services/nvpair-engine-manager/controlstream.go @@ -78,7 +78,9 @@ func (s *controlServer) handleInstall(w http.ResponseWriter, r *http.Request) { return } s.streamOp(w, r, req.OpID, req.Engine, "install", func(ctx context.Context) (streamFrame, error) { - if err := s.exec.Install(ctx, req.Engine); err != nil { + // A pinned peer may install here; it may not rewrite this user's login + // shell configuration. See Executor.InstallForPeer. + if err := s.exec.InstallForPeer(ctx, req.Engine); err != nil { return streamFrame{}, err } if req.Start { @@ -90,6 +92,7 @@ func (s *controlServer) handleInstall(w http.ResponseWriter, r *http.Request) { if err != nil { return streamFrame{}, err } + st.PathManaged = false // local account configuration; see handleEngines return streamFrame{Type: "result", OpID: req.OpID, Engine: req.Engine, Op: "install", Status: &st}, nil }) } diff --git a/services/nvpair-engine-manager/e2e_test.go b/services/nvpair-engine-manager/e2e_test.go index e02187a0..bcd3853c 100644 --- a/services/nvpair-engine-manager/e2e_test.go +++ b/services/nvpair-engine-manager/e2e_test.go @@ -7,6 +7,7 @@ import ( "bufio" "encoding/json" "io" + "maps" settings "nvpair-shared/enginesettings" "os" "os/exec" @@ -34,7 +35,7 @@ func TestE2EOverStdio(t *testing.T) { } cmd := exec.Command(managerBin) - cmd.Env = overrideEnv(map[string]string{"APPDATA": cfg, "LOCALAPPDATA": cfg, "XDG_CONFIG_HOME": cfg, "HOME": home}) + cmd.Env = sandboxedEnv(t, map[string]string{"APPDATA": cfg, "LOCALAPPDATA": cfg, "XDG_CONFIG_HOME": cfg, "HOME": home}) stdin, err := cmd.StdinPipe() if err != nil { t.Fatal(err) @@ -244,7 +245,7 @@ type e2eManager struct { func startE2EManager(t *testing.T, cfg, home string) *e2eManager { t.Helper() cmd := exec.Command(managerBin) - cmd.Env = overrideEnv(map[string]string{"APPDATA": cfg, "LOCALAPPDATA": cfg, "XDG_CONFIG_HOME": cfg, "HOME": home}) + cmd.Env = sandboxedEnv(t, map[string]string{"APPDATA": cfg, "LOCALAPPDATA": cfg, "XDG_CONFIG_HOME": cfg, "HOME": home}) stdin, err := cmd.StdinPipe() if err != nil { t.Fatal(err) @@ -308,6 +309,22 @@ func writeE2EManifest(t *testing.T, dir string, manifest *Manifest) { } } +// sandboxedEnv isolates a spawned manager from the developer's own account. +// +// Installing an engine publishes its CLI directory on the user's PATH, which +// means writing the profiles under HOME and choosing them from SHELL. Without +// this, an opt-in live run would append a block to the real ~/.zshrc pointing at +// a directory the test deletes on the way out — and any t.Fatalf before the +// uninstall step would leave it there for good. Explicit values win, so a caller +// that already points HOME at a temporary directory keeps its own. +func sandboxedEnv(t *testing.T, over map[string]string) []string { + t.Helper() + sandbox := t.TempDir() + env := map[string]string{"HOME": sandbox, "USERPROFILE": sandbox, "SHELL": "/bin/sh"} + maps.Copy(env, over) + return overrideEnv(env) +} + // overrideEnv returns the current environment with the given keys // replaced (case-insensitively, for Windows %AppData%). func overrideEnv(over map[string]string) []string { diff --git a/services/nvpair-engine-manager/executor.go b/services/nvpair-engine-manager/executor.go index de6bcfdd..08687305 100644 --- a/services/nvpair-engine-manager/executor.go +++ b/services/nvpair-engine-manager/executor.go @@ -35,6 +35,10 @@ type EngineStatus struct { Running bool `json:"running"` Healthy bool `json:"healthy"` Port int `json:"port,omitempty"` + // PathManaged is true while PAIR owns a PATH entry for this engine on this + // machine, so a client asks about removing it only when there is one. Always + // false in what the control surface serves to peers. + PathManaged bool `json:"path_managed"` } // engineState is the per-engine runtime state. @@ -93,6 +97,9 @@ type Executor struct { // detectTimeout bounds the post-install/uninstall detect poll // (installers finish their file work asynchronously). Overridable. detectTimeout time.Duration + // addToPath persists the CLI directory in the current user's PATH. + addToPath func(string, *pathReceipt, func() error) error + removeFromPath func(*pathReceipt) error // actionTimeout bounds a single engine:action call (HTTP or CLI) so a // hung engine can't park the goroutine or starve the caller forever. actionTimeout time.Duration @@ -124,6 +131,8 @@ func NewExecutor(reg *Registry, reporter *Reporter, emit func(string, any), base baseDir: baseDir, desired: newDesiredStateStore(baseDir), detectTimeout: 30 * time.Second, + addToPath: addToUserPath, + removeFromPath: removeUserPath, actionTimeout: 30 * time.Minute, loadedPollInterval: defaultLoadedPollSeconds * time.Second, loadedPoke: make(chan struct{}, 1), @@ -272,6 +281,10 @@ func expandPathForOS(s, goos string) string { func installFailedID(engine string) string { return "engine-manager:install-failed:" + engine } +// pathFailedID is deliberately separate from install-failed: the engine did +// install, so the two states have to be able to coexist and clear independently. +func pathFailedID(engine string) string { return "engine-manager:path-failed:" + engine } + func uninstallFailedID(engine string) string { return "engine-manager:uninstall-failed:" + engine } func pullFailedID(engine, model string) string { diff --git a/services/nvpair-engine-manager/executor_test.go b/services/nvpair-engine-manager/executor_test.go index 40ebe249..79aa0db7 100644 --- a/services/nvpair-engine-manager/executor_test.go +++ b/services/nvpair-engine-manager/executor_test.go @@ -56,7 +56,10 @@ func newTestExecutor(t *testing.T, m *Manifest) *Executor { t.Helper() reg := NewRegistry() reg.engines[m.Engine] = m - return NewExecutor(reg, NewReporter(nil), func(string, any) {}, t.TempDir()) + ex := NewExecutor(reg, NewReporter(nil), func(string, any) {}, t.TempDir()) + // Lifecycle tests must not modify the developer's persistent PATH. + ex.addToPath = func(string, *pathReceipt, func() error) error { return nil } + return ex } func responseHeaderTimeout(t *testing.T, client *http.Client) time.Duration { @@ -403,8 +406,9 @@ func TestInstallAdoptsExternalServiceWithoutDownloading(t *testing.T) { reg.engines[m.Engine] = m var methods []string ex := NewExecutor(reg, NewReporter(nil), func(method string, _ any) { methods = append(methods, method) }, t.TempDir()) + ex.addToPath = func(string, *pathReceipt, func() error) error { return nil } - if err := ex.Install(context.Background(), m.Engine); err != nil { + if err := ex.Install(context.Background(), m.Engine, true); err != nil { t.Fatalf("install should adopt the external service: %v", err) } if got := downloads.Load(); got != 0 { @@ -456,7 +460,7 @@ func TestInstallDoesNotOverwriteUnknownListener(t *testing.T) { }}, } ex := newTestExecutor(t, m) - err := ex.Install(context.Background(), m.Engine) + err := ex.Install(context.Background(), m.Engine, true) if err == nil || !strings.Contains(err.Error(), "occupied") { t.Fatalf("install over an unknown listener error = %v, want occupied", err) } @@ -498,7 +502,7 @@ func TestCommandModeMissingCLIInstallsDespiteLiveAPI(t *testing.T) { if st, err := ex.Status(m.Engine); err != nil || st.Installed || st.Running { t.Fatalf("live API without CLI status = %+v, err=%v; want not installed", st, err) } - if err := ex.Install(context.Background(), m.Engine); err != nil { + if err := ex.Install(context.Background(), m.Engine, true); err != nil { t.Fatalf("install missing command-mode CLI: %v", err) } if !fileExists(cli) { @@ -605,7 +609,7 @@ func TestProxyFacadeCannotIdentifyAsOllama(t *testing.T) { if st, err := ex.Status("ollama"); err != nil || st.Installed || st.Running || st.Healthy { t.Fatalf("proxy facade was adopted: status=%+v err=%v", st, err) } - if err := ex.Install(context.Background(), "ollama"); err == nil || !strings.Contains(err.Error(), "occupied") { + if err := ex.Install(context.Background(), "ollama", true); err == nil || !strings.Contains(err.Error(), "occupied") { t.Fatalf("install over proxy facade error = %v", err) } if err := ex.Start(context.Background(), "ollama"); err == nil || !strings.Contains(err.Error(), "occupied") { @@ -662,7 +666,7 @@ func TestPreviouslyAdoptedForeignReplacementFailsClosed(t *testing.T) { if err := ex.Start(context.Background(), "ollama"); err == nil || !strings.Contains(err.Error(), "occupied") { t.Fatalf("Start accepted foreign replacement: %v", err) } - if err := ex.Install(context.Background(), "ollama"); err == nil || !strings.Contains(err.Error(), "occupied") { + if err := ex.Install(context.Background(), "ollama", true); err == nil || !strings.Contains(err.Error(), "occupied") { t.Fatalf("Install accepted foreign replacement: %v", err) } } @@ -702,7 +706,7 @@ func TestUninstallTerminatesRunningInstance(t *testing.T) { reg := NewRegistry() reg.engines[m.Engine] = m ex := NewExecutor(reg, NewReporter(nil), func(string, any) {}, baseDir) - if err := ex.Uninstall(context.Background(), "fake"); err != nil { + if err := ex.Uninstall(context.Background(), "fake", true); err != nil { t.Fatalf("uninstall: %v", err) } if ok, _ := ex.Detect("fake"); ok { @@ -749,7 +753,7 @@ func TestUninstallDoesNotKillExternalSameNameProcess(t *testing.T) { reg.engines[m.Engine] = m ex := NewExecutor(reg, NewReporter(nil), func(string, any) {}, baseDir) - err = ex.Uninstall(context.Background(), m.Engine) + err = ex.Uninstall(context.Background(), m.Engine, true) if err == nil || !strings.Contains(err.Error(), "external management") { t.Fatalf("uninstall external same-name owner error = %v", err) } @@ -772,7 +776,7 @@ func TestUninstallRefusesExternalProcessEngine(t *testing.T) { m.Platforms[key] = p ex := newTestExecutor(t, m) // managed base is a different temp directory - err := ex.Uninstall(context.Background(), m.Engine) + err := ex.Uninstall(context.Background(), m.Engine, true) if err == nil || !strings.Contains(err.Error(), "outside NVPAIR's managed install directory") { t.Fatalf("external uninstall error = %v", err) } @@ -806,7 +810,7 @@ func TestUninstallRefusesUnidentifiedLiveListener(t *testing.T) { reg := NewRegistry() reg.engines[m.Engine] = m ex := NewExecutor(reg, NewReporter(nil), func(string, any) {}, baseDir) - err := ex.Uninstall(context.Background(), m.Engine) + err := ex.Uninstall(context.Background(), m.Engine, true) if err == nil || !strings.Contains(err.Error(), "occupied by an unidentified service") { t.Fatalf("uninstall unidentified listener error = %v", err) } @@ -884,7 +888,7 @@ func TestInstallRefusesAdmin(t *testing.T) { m.Platforms[key] = p ex := newTestExecutor(t, m) - err := ex.Install(context.Background(), "fake") + err := ex.Install(context.Background(), "fake", true) if err == nil || !strings.Contains(err.Error(), "refused") { t.Fatalf("expected admin-install refusal, got %v", err) } @@ -913,7 +917,7 @@ func TestEngineUninstall(t *testing.T) { if ok, _ := ex.Detect("fake"); !ok { t.Fatal("expected installed before uninstall") } - if err := ex.Uninstall(context.Background(), "fake"); err != nil { + if err := ex.Uninstall(context.Background(), "fake", true); err != nil { t.Fatalf("uninstall: %v", err) } if ok, _ := ex.Detect("fake"); ok { @@ -924,7 +928,7 @@ func TestEngineUninstall(t *testing.T) { func TestUninstallNoOpWhenAbsent(t *testing.T) { m := testEngineManifest(filepath.Join(t.TempDir(), "absent"+exeExt())) ex := newTestExecutor(t, m) - if err := ex.Uninstall(context.Background(), "fake"); err != nil { + if err := ex.Uninstall(context.Background(), "fake", true); err != nil { t.Fatalf("uninstall of an absent engine should be a no-op, got %v", err) } } diff --git a/services/nvpair-engine-manager/install.go b/services/nvpair-engine-manager/install.go index 268bd0b9..7f68aad2 100644 --- a/services/nvpair-engine-manager/install.go +++ b/services/nvpair-engine-manager/install.go @@ -7,6 +7,7 @@ import ( "context" "crypto/sha256" "encoding/hex" + "errors" "fmt" "io" "log/slog" @@ -15,13 +16,66 @@ import ( "os" "os/exec" "path" + "path/filepath" "strings" "time" ) -// Install obtains the engine in user mode: download (checksum-verified) -// then run the declared command. No-op if already detected. -func (e *Executor) Install(ctx context.Context, engine string) error { +// installPathMode says what an install may do to the user's PATH. +type installPathMode int + +const ( + // pathRecordOnly records that PAIR ran the installer without publishing + // anything, so a later consented install can still re-adopt an engine whose + // vendor owns its location. + pathRecordOnly installPathMode = iota + // pathPublish publishes the CLI directory on the user's PATH. + pathPublish +) + +// uninstallPathMode says what an uninstall does with the PATH entries PAIR +// published for the engine. +type uninstallPathMode int + +const ( + // pathKeepClaim is an uninstall whose client did not ask the user. The + // entries stay, and so does PAIR's claim on them, so a later uninstall or the + // application uninstaller can still remove them. + pathKeepClaim uninstallPathMode = iota + // pathRelease is a user who kept the entries: they stay, and the record that + // claims them goes, so they are the user's from here on. + pathRelease + // pathRemove is a user who asked for the entries to go. + pathRemove +) + +// Install obtains the engine in user mode for a request that originated on this +// machine: download (checksum-verified), run the declared command, then, when +// the user consented, publish the CLI directory on this user's PATH. +func (e *Executor) Install(ctx context.Context, engine string, addToPath bool) error { + if addToPath { + return e.install(ctx, engine, pathPublish) + } + return e.install(ctx, engine, pathRecordOnly) +} + +// InstallForPeer installs on behalf of a paired cluster node. +// +// It is the same install with the PATH step withheld. A peer may put an engine +// on this machine, but editing this user's login shell configuration and +// HKCU\Environment is a different kind of change: the pairing PIN is a +// convenience code, cluster membership is not proof of a vetted peer, and +// nothing in the remote-install flow tells the person sitting at this machine +// that their shell startup files are about to be rewritten. +// +// It still records that PAIR ran the installer. That publishes nothing, and it +// is what lets a later consented local install adopt an engine whose vendor +// owns its location. +func (e *Executor) InstallForPeer(ctx context.Context, engine string) error { + return e.install(ctx, engine, pathRecordOnly) +} + +func (e *Executor) install(ctx context.Context, engine string, pathMode installPathMode) error { st, err := e.state(engine) if err != nil { return err @@ -29,8 +83,18 @@ func (e *Executor) Install(ctx context.Context, engine string) error { st.opMu.Lock() defer st.opMu.Unlock() if ok, _ := e.Detect(engine); ok { + // Nothing was installed now, so a declined install records nothing: + // the engine on disk is no evidence that PAIR placed it. + if pathMode == pathPublish { + e.installPath(engine, st, false) + } e.reporter.clear(installFailedID(engine)) e.emitInstallProgress(engine, "already-installed", 100) + if pathMode == pathPublish { + // Nothing else about the engine changed, so without this no client + // learns that PAIR now owns a PATH entry it may later offer to remove. + e.emitState(engine) + } return nil } st.mu.Lock() @@ -65,6 +129,7 @@ func (e *Executor) Install(ctx context.Context, engine string) error { } vars := map[string]string{"install_dir": st.installDir} + installEnv := inst.environ() if len(inst.Script) > 0 { // Escape hatch: vendor-script install with no checksum. Logged @@ -79,7 +144,7 @@ func (e *Executor) Install(ctx context.Context, engine string) error { for i := range argv { argv[i] = expandPath(argv[i]) } - if err := e.runCommand(ctx, argv); err != nil { + if err := e.runCommand(ctx, argv, installEnv...); err != nil { werr := fmt.Errorf("script install failed: %w", err) e.reportInstallFailed(engine, werr) return werr @@ -106,7 +171,7 @@ func (e *Executor) Install(ctx context.Context, engine string) error { for i := range args { args[i] = expandPath(args[i]) } - if err := e.runCommand(ctx, args); err != nil { + if err := e.runCommand(ctx, args, installEnv...); err != nil { werr := fmt.Errorf("install command failed: %w", err) e.reportInstallFailed(engine, werr) return werr @@ -119,15 +184,118 @@ func (e *Executor) Install(ctx context.Context, engine string) error { e.reportInstallFailed(engine, err) return err } + switch pathMode { + case pathPublish: + e.installPath(engine, st, true) + case pathRecordOnly: + if err := e.recordInstalled(engine); err != nil { + slog.Warn("could not record that PAIR installed an engine", "engine", engine, "err", err) + } + } e.reporter.clear(installFailedID(engine)) e.emitInstallProgress(engine, "done", 100) e.emitState(engine) return nil } +// installPath publishes the engine's command-line directory on the user's PATH. +// +// A PATH problem is a warning, never an install failure. The engine is +// installed and fully usable without it, and the caller gates the optional +// start step on install succeeding — so failing here would leave a working +// engine stopped, carrying an error card, behind a retry that clears nothing. +func (e *Executor) installPath(engine string, st *engineState, installedNow bool) { + dir, err := e.updateInstallPath(engine, st, installedNow) + if err != nil { + e.reportPathFailed(engine, st.manifest.DisplayName, dir, err) + return + } + if dir == "" { + // An external install PAIR declined to touch. Nothing was published, so + // there is nothing to report cleared: clearing here would retract a + // standing warning about an entry still missing from the user's PATH. + return + } + e.reporter.clear(pathFailedID(engine)) +} + +// pathCLIDir resolves the directory to publish on PATH. +// +// The manifest's declared runtime.cli is authoritative. A detect entry is only +// a fallback and only from inside the engine's managed install directory: +// detect deliberately matches vendor installs PAIR does not own (Ollama's +// darwin list starts at /Applications/Ollama.app), and its first hit is a +// discovery result, not a statement about where the CLI lives. +func pathCLIDir(st *engineState) (string, error) { + var cli string + if declared := st.plat.Runtime.CLI; declared != "" { + // {install_dir} is a documented runtime placeholder that detect and + // runtime.bin both resolve, so PATH has to resolve it too. Without this + // a manifest written to MANIFEST.md produced a non-absolute path and + // published nothing but a warning. + resolved, err := resolvePlaceholders(declared, map[string]string{"install_dir": st.installDir}) + if err != nil { + return "", err + } + cli = expandPath(resolved) + } else { + if !managedCLI(st) { + return "", fmt.Errorf("its command-line executable is outside the directory PAIR installs into") + } + // Detect has already expanded binPath; expanding it again would corrupt + // a directory that legitimately contains a $ or %. + st.mu.Lock() + cli = st.binPath + st.mu.Unlock() + } + if !filepath.IsAbs(cli) { + // Anchoring this to the daemon's working directory would produce a PATH + // entry that means nothing; a manifest has to declare an absolute path. + return "", fmt.Errorf("the manifest resolves its command-line executable to a relative path") + } + // An unreadable executable is not an absent one. Collapsing the two told the + // user to reinstall over a permission or I/O error, which fails identically. + if _, err := os.Stat(cli); err != nil { + if errors.Is(err, os.ErrNotExist) { + return "", fmt.Errorf("its command-line executable was not found") + } + return "", fmt.Errorf("its command-line executable could not be read: %w", err) + } + return filepath.Dir(cli), nil +} + +// managedCLI reports whether the detected executable is one PAIR placed. An +// engine whose vendor installer owns its own location (LM Studio writes +// ~/.lmstudio) is never managed by this test, so a lost receipt there is +// indistinguishable from an external install and is left alone. +func managedCLI(st *engineState) bool { + st.mu.Lock() + bin := st.binPath + st.mu.Unlock() + return isManagedInstallPath(bin, st.installDir) +} + // Uninstall runs the manifest's uninstall command (user-mode), stopping // the engine first. No-op if the engine isn't currently detected. -func (e *Executor) Uninstall(ctx context.Context, engine string) error { +// +// removePath is the user's answer to removing what PAIR added to PATH. When +// false the entries stay and PAIR gives up its claim on them, so they become +// the user's and the application uninstaller leaves them alone too. +func (e *Executor) Uninstall(ctx context.Context, engine string, removePath bool) error { + if removePath { + return e.uninstall(ctx, engine, pathRemove) + } + return e.uninstall(ctx, engine, pathRelease) +} + +// UninstallKeepingPathClaim uninstalls for a client that did not ask the user +// about PATH. Only an answer may remove the entries or hand them over, so both +// stay exactly as they are. +func (e *Executor) UninstallKeepingPathClaim(ctx context.Context, engine string) error { + return e.uninstall(ctx, engine, pathKeepClaim) +} + +func (e *Executor) uninstall(ctx context.Context, engine string, pathMode uninstallPathMode) error { st, err := e.state(engine) if err != nil { return err @@ -135,7 +303,12 @@ func (e *Executor) Uninstall(ctx context.Context, engine string) error { st.opMu.Lock() defer st.opMu.Unlock() if ok, _ := e.Detect(engine); !ok { - return e.setDesiredEnabled(engine, false) // already gone + // Joined rather than sequenced: this branch is the documented retry + // after the files are already gone, so a desired-state write that keeps + // failing must not be what makes PATH cleanup unreachable. + err := errors.Join(e.setDesiredEnabled(engine, false), e.finishUninstallPath(engine, pathMode)) + e.emitState(engine) + return err } un := st.plat.Uninstall if un == nil || len(un.Run) == 0 { @@ -204,9 +377,24 @@ func (e *Executor) Uninstall(ctx context.Context, engine string) error { st.mu.Lock() st.binPath = "" st.mu.Unlock() - e.reporter.clear(uninstallFailedID(engine)) + // The engine is gone either way, so both steps run and both errors travel. + // Short-circuiting here left PATH published and the previous attempt's + // error card standing whenever the desired-state write failed. + err = errors.Join(e.setDesiredEnabled(engine, false), e.finishUninstallPath(engine, pathMode)) + // Emitted after the PATH step so the state carries the final path_managed. e.emitState(engine) - return e.setDesiredEnabled(engine, false) + return err +} + +func (e *Executor) finishUninstallPath(engine string, pathMode uninstallPathMode) error { + switch pathMode { + case pathRemove: + return e.uninstallPath(engine) + case pathRelease: + return e.releasePathOwnership(engine) + default: + return e.keepPathClaim(engine) + } } // maxDownloadBytes caps a single engine download (engine installers / @@ -309,11 +497,14 @@ func (e *Executor) download(ctx context.Context, engine string, f *Fetch) (strin // runCommand executes a manifest-declared argv (an install or uninstall // step), hiding the console window on Windows; on failure it returns the // combined output for diagnostics. -func (e *Executor) runCommand(ctx context.Context, argv []string) error { +func (e *Executor) runCommand(ctx context.Context, argv []string, env ...string) error { if len(argv) == 0 { return nil } cmd := exec.CommandContext(ctx, argv[0], argv[1:]...) + if len(env) > 0 { + cmd.Env = append(os.Environ(), env...) + } configureSysProcAttr(cmd) // hide the console window on Windows out, err := cmd.CombinedOutput() if err != nil { @@ -322,6 +513,34 @@ func (e *Executor) runCommand(ctx context.Context, argv []string) error { return nil } +// reportPathFailed surfaces a PATH problem as a dismissible warning telling the +// user what to do instead. +// +// The reported text is streamed to a remote install's initiator and push-synced +// to cluster peers, so the home directory is folded back to "~" before it +// leaves. That keeps the account name off the wire while still naming the +// profile the user has to edit. The full paths stay in this node's log. +func (e *Executor) reportPathFailed(engine, displayName, dir string, err error) { + slog.Warn("could not add an engine's command-line tools to PATH", + "engine", engine, "directory", dir, "err", err) + message := fmt.Sprintf( + "%s is installed and ready, but its command-line tools could not be added to your PATH (%s). Run the engine using its full path, or add its directory to your PATH yourself — the service log names the directory.", + displayName, redactHome(err.Error())) + e.reporter.report(serviceError{ + ID: pathFailedID(engine), Message: message, + Severity: "warning", Action: "dismiss", EngineType: engine, Operation: "install", + }) +} + +// redactHome keeps the user's home directory out of text that leaves this node. +func redactHome(text string) string { + home, err := os.UserHomeDir() + if err != nil || home == "" { + return text + } + return strings.ReplaceAll(text, home, "~") +} + func (e *Executor) reportInstallFailed(engine string, err error) { e.notify("engine:install-progress", map[string]any{"engine": engine, "stage": "failed", "percent": -1, "error": err.Error()}) e.progress.publish(ProgressEvent{Engine: engine, Op: "install", Stage: "failed", Percent: -1, Message: err.Error()}) diff --git a/services/nvpair-engine-manager/install_path_test.go b/services/nvpair-engine-manager/install_path_test.go new file mode 100644 index 00000000..3d46de58 --- /dev/null +++ b/services/nvpair-engine-manager/install_path_test.go @@ -0,0 +1,605 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +package main + +import ( + "bytes" + "context" + "encoding/json" + "errors" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "runtime" + "strings" + "sync" + "testing" +) + +// pathInstallExecutor builds an engine whose detect path and declared CLI are +// deliberately different files inside one managed install directory. Detect is a +// discovery hint that matches whatever layout a vendor ships; only runtime.cli +// names the directory PAIR is entitled to publish on PATH. +func pathInstallExecutor(t *testing.T, engine, mode string) (*Executor, string) { + t.Helper() + installDir := filepath.Join(t.TempDir(), "Engine Tools") + detected := filepath.Join(installDir, engine+exeExt()) + cli := filepath.Join(installDir, "bin", engine+exeExt()) + if err := os.MkdirAll(filepath.Dir(cli), 0o755); err != nil { + t.Fatal(err) + } + m := &Manifest{ + Engine: engine, DisplayName: engine, ManifestVersion: 1, + Platforms: map[string]Platform{runtime.GOOS + "/" + runtime.GOARCH: { + Detect: []string{detected}, + Install: &Install{Run: []string{fakeEngineBin, "touch", detected, cli}}, + Runtime: Runtime{Mode: mode, Bin: detected, CLI: cli}, + }}, + } + ex := newTestExecutor(t, m) + engineStateForTest(t, ex, engine).installDir = installDir + return ex, cli +} + +func engineStateForTest(t *testing.T, ex *Executor, engine string) *engineState { + t.Helper() + st, err := ex.state(engine) + if err != nil { + t.Fatal(err) + } + return st +} + +// pathWarning returns the reported PATH warning for an engine, or "". +func pathWarning(ex *Executor, engine string) string { + for _, e := range ex.reporter.snapshot() { + if e.ID == pathFailedID(engine) { + return e.Message + } + } + return "" +} + +func TestConsentedInstallAddsCLIToPath(t *testing.T) { + test := func(name, engine, mode string) { + t.Run(name, func(t *testing.T) { + ex, cli := pathInstallExecutor(t, engine, mode) + var added []string + ex.addToPath = func(dir string, _ *pathReceipt, _ func() error) error { + if !fileExists(cli) { + t.Error("PATH changed before CLI was installed") + } + added = append(added, dir) + return nil + } + if err := ex.Install(context.Background(), engine, true); err != nil { + t.Fatal(err) + } + if !fileExists(cli) { + t.Fatal("engine was not installed") + } + if len(added) != 1 || added[0] != filepath.Dir(cli) { + t.Fatalf("added directories = %v", added) + } + }) + } + test("Ollama CLI directory", "ollama", "process") + test("LM Studio CLI directory", "lmstudio", "command") +} + +// A PATH failure must not fail the install. Failing it would also skip the +// caller's start step, so a working engine would sit stopped behind an error. +func TestPathFailureStillCompletesInstallAndWarns(t *testing.T) { + ex, cli := pathInstallExecutor(t, "ollama", "process") + var announced EngineStatus + ex.emit = func(method string, params any) { + if method != "engine:state-changed" { + return + } + data, err := json.Marshal(params) + if err != nil { + t.Fatal(err) + } + if err := json.Unmarshal(data, &announced); err != nil { + t.Fatal(err) + } + } + ex.addToPath = func(string, *pathReceipt, func() error) error { + return errors.New("profile is read only") + } + if err := ex.Install(context.Background(), "ollama", true); err != nil { + t.Fatalf("install error = %v, want success with a PATH warning", err) + } + installed, err := ex.Detect("ollama") + if err != nil || !installed { + t.Fatalf("installed = %v, error = %v", installed, err) + } + if !announced.Installed { + t.Fatal("UI was not told the engine remains installed") + } + warning := pathWarning(ex, "ollama") + if !strings.Contains(warning, "profile is read only") || !strings.Contains(warning, "full path") { + t.Fatalf("PATH warning = %q, want the reason and the manual alternative", warning) + } + + var added string + ex.addToPath = func(dir string, _ *pathReceipt, _ func() error) error { added = dir; return nil } + if err := ex.Install(context.Background(), "ollama", true); err != nil { + t.Fatal(err) + } + if added != filepath.Dir(cli) { + t.Fatalf("retry added %q", added) + } + if warning := pathWarning(ex, "ollama"); warning != "" { + t.Fatalf("retry left the warning in place: %q", warning) + } +} + +// The warning crosses the wire to a remote initiator and is push-synced to +// cluster peers, so it must not carry the user's home directory. +func TestPathWarningOmitsTheUsersHomeDirectory(t *testing.T) { + home, err := os.UserHomeDir() + if err != nil || home == "" { + t.Skip("no home directory to redact") + } + ex, _ := pathInstallExecutor(t, "ollama", "process") + ex.addToPath = func(string, *pathReceipt, func() error) error { + return errors.New("open " + filepath.Join(home, ".zshrc") + ": permission denied") + } + if err := ex.Install(context.Background(), "ollama", true); err != nil { + t.Fatal(err) + } + warning := pathWarning(ex, "ollama") + if strings.Contains(warning, home) { + t.Fatalf("PATH warning leaked the home directory: %q", warning) + } + if !strings.Contains(warning, ".zshrc") { + t.Fatalf("PATH warning dropped the profile name: %q", warning) + } +} + +// A detect entry can match a vendor layout PAIR does not own, so it is never +// enough on its own to claim a PATH directory. PAIR having just installed the +// engine is what separates this from an ordinary pre-existing installation. +func TestPathRefusesAnUndeclaredCLIOutsideTheInstallDirectory(t *testing.T) { + ex, cli := pathInstallExecutor(t, "ollama", "process") + st := engineStateForTest(t, ex, "ollama") + st.plat.Runtime.CLI = "" + st.installDir = t.TempDir() + ex.addToPath = func(string, *pathReceipt, func() error) error { + t.Error("claimed a PATH directory PAIR does not own") + return nil + } + if err := ex.Install(context.Background(), "ollama", true); err != nil { + t.Fatal(err) + } + if !fileExists(cli) { + t.Fatal("engine was not installed") + } + if warning := pathWarning(ex, "ollama"); !strings.Contains(warning, "outside the directory PAIR installs into") { + t.Fatalf("PATH warning = %q", warning) + } +} + +// A user who already had the engine before PAIR gets no PATH entry and no +// warning: the vendor's installer owns that location and its PATH. +func TestPreexistingEngineProducesNoPathWarning(t *testing.T) { + ex, cli := pathInstallExecutor(t, "ollama", "process") + st := engineStateForTest(t, ex, "ollama") + st.plat.Runtime.CLI = "" + st.installDir = t.TempDir() + for _, path := range append([]string{cli}, st.plat.Detect...) { + if err := os.WriteFile(path, []byte("vendor install"), 0o644); err != nil { + t.Fatal(err) + } + } + ex.addToPath = func(string, *pathReceipt, func() error) error { + t.Error("claimed a PATH directory PAIR does not own") + return nil + } + if err := ex.Install(context.Background(), "ollama", true); err != nil { + t.Fatal(err) + } + if warning := pathWarning(ex, "ollama"); warning != "" { + t.Fatalf("warned about a PATH entry PAIR was never going to add: %q", warning) + } +} + +// A relative path is meaningless as a PATH entry: it would resolve against the +// daemon's working directory, which is not where the engine lives. +func TestPathRejectsARelativeManifestCLI(t *testing.T) { + ex, cli := pathInstallExecutor(t, "ollama", "process") + st := engineStateForTest(t, ex, "ollama") + t.Chdir(filepath.Dir(cli)) + st.plat.Runtime.CLI = filepath.Base(cli) + ex.addToPath = func(string, *pathReceipt, func() error) error { + t.Error("published a PATH entry from a relative manifest path") + return nil + } + if err := ex.Install(context.Background(), "ollama", true); err != nil { + t.Fatal(err) + } + if warning := pathWarning(ex, "ollama"); !strings.Contains(warning, "relative path") { + t.Fatalf("PATH warning = %q", warning) + } +} + +// The runner applies whatever a manifest declares, so a third-party engine can +// suppress its installer's own PATH edits without a code change. +func TestInstallAppliesManifestDeclaredInstallerEnvironment(t *testing.T) { + t.Setenv("PAIR_TEST_INSTALL_ENV", "inherited") + ex, cli := pathInstallExecutor(t, "ollama", "process") + st := engineStateForTest(t, ex, "ollama") + record := filepath.Join(filepath.Dir(cli), "installer-env") + st.plat.Install.Env = map[string]string{"PAIR_TEST_INSTALL_ENV": "declared"} + st.plat.Install.Run = []string{fakeEngineBin, "write-env", "PAIR_TEST_INSTALL_ENV", record} + st.plat.Detect = []string{record} + if err := ex.Install(context.Background(), "ollama", true); err != nil { + t.Fatal(err) + } + value, err := os.ReadFile(record) + if err != nil { + t.Fatal(err) + } + if string(value) != "declared" { + t.Fatalf("installer saw %q, want the manifest override", value) + } + if os.Getenv("PAIR_TEST_INSTALL_ENV") != "inherited" { + t.Fatal("the override escaped into the manager's own environment") + } +} + +// The bundled LM Studio manifest — not a branch in the runner — is what keeps +// the vendor installer from writing PATH entries PAIR cannot clean up. +func TestBundledLMStudioManifestSuppressesVendorPathEdits(t *testing.T) { + reg := NewRegistry() + if err := reg.LoadFS(bundledManifests, "manifests"); err != nil { + t.Fatal(err) + } + m, ok := reg.Get("lmstudio") + if !ok { + t.Fatal("lmstudio manifest is not bundled") + } + for key, plat := range m.Platforms { + if plat.Install == nil { + continue + } + if plat.Install.Env["LMS_NO_MODIFY_PATH"] != "1" { + t.Errorf("%s install env = %v", key, plat.Install.Env) + } + } +} + +func TestFailedInstallDoesNotChangePath(t *testing.T) { + ex, _ := pathInstallExecutor(t, "ollama", "process") + st, err := ex.state("ollama") + if err != nil { + t.Fatal(err) + } + st.plat.Install.Run = []string{filepath.Join(t.TempDir(), "missing-installer")} + ex.addToPath = func(string, *pathReceipt, func() error) error { + t.Error("PATH changed after failed install") + return nil + } + if err := ex.Install(context.Background(), "ollama", true); err == nil { + t.Fatal("expected install failure") + } +} + +// runOpForTest sends one lifecycle request through the JSON-RPC front end and +// returns the status it responded with. +func runOpForTest(t *testing.T, ex *Executor, method, params string) EngineStatus { + t.Helper() + var out bytes.Buffer + m := NewManager(NewCodec(&out), ex, nil) + id := json.RawMessage("1") + m.runOp(context.Background(), &Message{JSONRPC: "2.0", ID: &id, Method: method, Params: json.RawMessage(params)}) + var response struct { + Result EngineStatus `json:"result"` + Error *json.RawMessage `json:"error"` + } + if err := json.Unmarshal(out.Bytes(), &response); err != nil { + t.Fatal(err) + } + if response.Error != nil { + t.Fatalf("%s returned an error: %s", method, *response.Error) + } + return response.Result +} + +func TestInstallRPCAddsCLIToPathOnlyWithConsent(t *testing.T) { + t.Run("path true publishes the CLI directory", func(t *testing.T) { + ex, cli := pathInstallExecutor(t, "ollama", "process") + var added string + ex.addToPath = func(dir string, receipt *pathReceipt, save func() error) error { + added = dir + receipt.ShellBlocks = []pathBlock{{Profile: "profile", Text: dir}} + return save() + } + status := runOpForTest(t, ex, "engine:install", `{"engine":"ollama","path":true}`) + if !status.Installed { + t.Error("install reported the engine as not installed") + } + if added != filepath.Dir(cli) { + t.Errorf("published %q on PATH, want the CLI directory %q", added, filepath.Dir(cli)) + } + if !status.PathManaged { + t.Error("status does not report the PATH entry PAIR now owns") + } + }) + t.Run("omitted path leaves PATH alone", func(t *testing.T) { + ex, cli := pathInstallExecutor(t, "ollama", "process") + ex.addToPath = func(string, *pathReceipt, func() error) error { + t.Error("changed PATH without the user's consent") + return nil + } + status := runOpForTest(t, ex, "engine:install", `{"engine":"ollama"}`) + if !status.Installed || !fileExists(cli) { + t.Fatal("a declined PATH entry must still install the engine") + } + if status.PathManaged { + t.Error("status claims a PATH entry that was never published") + } + }) +} + +// Declining the PATH entry still records that PAIR installed the engine, so a +// later consented install of the engine already on disk can publish it even +// when the vendor owns the engine's location. +func TestDeclinedInstallRecordsOwnershipForALaterConsent(t *testing.T) { + ex, cli := pathInstallExecutor(t, "lmstudio", "command") + st := engineStateForTest(t, ex, "lmstudio") + st.installDir = t.TempDir() + if err := ex.Install(context.Background(), "lmstudio", false); err != nil { + t.Fatal(err) + } + receipt, err := loadPathReceipt(ex.pathReceiptFile("lmstudio")) + if err != nil { + t.Fatal(err) + } + if !receipt.Installed || receipt.Dir != "" { + t.Fatalf("receipt after a declined install = %+v, want only the installed flag", receipt) + } + + var added string + ex.addToPath = func(dir string, _ *pathReceipt, _ func() error) error { added = dir; return nil } + if err := ex.Install(context.Background(), "lmstudio", true); err != nil { + t.Fatal(err) + } + if added != filepath.Dir(cli) { + t.Fatalf("consented reinstall published %q, want %q", added, filepath.Dir(cli)) + } +} + +// Installing over an engine already on disk without consent records nothing: +// finding the engine there is no evidence that PAIR put it there. +func TestDeclinedInstallOfAPresentEngineRecordsNothing(t *testing.T) { + ex, _ := pathInstallExecutor(t, "ollama", "process") + st := engineStateForTest(t, ex, "ollama") + for _, path := range st.plat.Detect { + if err := os.WriteFile(path, []byte("vendor install"), 0o644); err != nil { + t.Fatal(err) + } + } + if err := ex.Install(context.Background(), "ollama", false); err != nil { + t.Fatal(err) + } + if _, err := os.Stat(ex.pathReceiptFile("ollama")); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("a declined install over a present engine wrote a record: %v", err) + } +} + +func TestUninstallRPCRemovesPathOnlyWithConsent(t *testing.T) { + install := func(t *testing.T) (*Executor, string, string) { + ex, cli, profile := pathLifecycleExecutor(t, "ollama", "process") + if err := ex.Install(context.Background(), "ollama", true); err != nil { + t.Fatal(err) + } + return ex, filepath.Dir(cli), profile + } + profileHas := func(t *testing.T, profile, dir string) bool { + t.Helper() + data, err := os.ReadFile(profile) + if err != nil && !errors.Is(err, os.ErrNotExist) { + t.Fatal(err) + } + return strings.Contains(string(data), dir) + } + t.Run("path true removes the entry", func(t *testing.T) { + ex, dir, profile := install(t) + status := runOpForTest(t, ex, "engine:uninstall", `{"engine":"ollama","path":true}`) + if profileHas(t, profile, dir) { + t.Fatal("the consented uninstall left the PATH entry in the profile") + } + if status.PathManaged { + t.Error("status still claims the removed PATH entry") + } + }) + t.Run("path false keeps the entry and gives up the claim", func(t *testing.T) { + ex, dir, profile := install(t) + status := runOpForTest(t, ex, "engine:uninstall", `{"engine":"ollama","path":false}`) + if !profileHas(t, profile, dir) { + t.Fatal("removed the PATH entry although the user kept it") + } + if _, err := os.Stat(ex.pathReceiptFile("ollama")); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("the record claiming the kept entry survived: %v", err) + } + if status.PathManaged { + t.Error("status still claims an entry that now belongs to the user") + } + }) + // A client that never asked the user must not hand the entries over: that + // would leave them with no record the application uninstaller can act on. + t.Run("omitted path keeps the entry and the claim", func(t *testing.T) { + ex, dir, profile := install(t) + status := runOpForTest(t, ex, "engine:uninstall", `{"engine":"ollama"}`) + if !profileHas(t, profile, dir) { + t.Fatal("removed the PATH entry without the user's consent") + } + receipt, err := loadPathReceipt(ex.pathReceiptFile("ollama")) + if err != nil { + t.Fatal(err) + } + if !receipt.ownsEntries() || receipt.Installed { + t.Fatalf("receipt after an unasked uninstall = %+v, want the entries without the installed flag", receipt) + } + if !status.PathManaged { + t.Error("status dropped the claim PAIR still holds") + } + runOpForTest(t, ex, "engine:uninstall", `{"engine":"ollama","path":true}`) + if profileHas(t, profile, dir) { + t.Fatal("the kept claim could not remove the entry later") + } + }) +} + +// A recorded directory is not an entry. The directory may already have been on +// PATH, or the write may have failed, and there is then nothing to remove. +func TestPathManagedRequiresAnEntryPAIRWrote(t *testing.T) { + ex, _ := pathInstallExecutor(t, "ollama", "process") + file := ex.pathReceiptFile("ollama") + if err := savePathReceipt(file, &pathReceipt{Dir: t.TempDir(), Installed: true}); err != nil { + t.Fatal(err) + } + if ex.pathManaged("ollama") { + t.Error("a directory PAIR did not write an entry for counts as managed") + } + if err := savePathReceipt(file, &pathReceipt{Dir: "d", WindowsEntry: "d"}); err != nil { + t.Fatal(err) + } + if !ex.pathManaged("ollama") { + t.Error("an entry PAIR wrote does not count as managed") + } +} + +// The state a client renders its uninstall prompt from has to be the one after +// the PATH step, not before it. +func TestLifecycleStateCarriesTheFinalPathManaged(t *testing.T) { + // The ops below run synchronously, so every emit has landed before a read. + captureStates := func(ex *Executor) *[]EngineStatus { + var mu sync.Mutex + var states []EngineStatus + ex.emit = func(method string, params any) { + if status, ok := params.(EngineStatus); ok && method == "engine:state-changed" { + mu.Lock() + states = append(states, status) + mu.Unlock() + } + } + return &states + } + t.Run("uninstall", func(t *testing.T) { + ex, _, _ := pathLifecycleExecutor(t, "ollama", "process") + if err := ex.Install(context.Background(), "ollama", true); err != nil { + t.Fatal(err) + } + states := captureStates(ex) + if err := ex.Uninstall(context.Background(), "ollama", true); err != nil { + t.Fatal(err) + } + if len(*states) == 0 { + t.Fatal("uninstall emitted no state") + } + if last := (*states)[len(*states)-1]; last.PathManaged { + t.Error("the last state after uninstall still claims the removed entry") + } + }) + t.Run("consented install of an engine already present", func(t *testing.T) { + ex, _, _ := pathLifecycleExecutor(t, "ollama", "process") + if err := ex.Install(context.Background(), "ollama", false); err != nil { + t.Fatal(err) + } + states := captureStates(ex) + if err := ex.Install(context.Background(), "ollama", true); err != nil { + t.Fatal(err) + } + if len(*states) == 0 || !(*states)[len(*states)-1].PathManaged { + t.Errorf("states after adopting the engine onto PATH = %+v, want one reporting the entry", *states) + } + }) +} + +// A consented uninstall removes the whole record, including a declined +// install's installed flag, so a later vendor install is not mistaken for PAIR's. +func TestConsentedUninstallDropsAnInstalledOnlyRecord(t *testing.T) { + ex, _, _ := pathLifecycleExecutor(t, "ollama", "process") + if err := ex.Install(context.Background(), "ollama", false); err != nil { + t.Fatal(err) + } + if err := ex.Uninstall(context.Background(), "ollama", true); err != nil { + t.Fatal(err) + } + if _, err := os.Stat(ex.pathReceiptFile("ollama")); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("the installed flag outlived the engine: %v", err) + } +} + +// A pinned peer may install an engine here, but rewriting this user's login +// shell configuration is a different kind of change and stays local-only. +func TestRemoteInstallDoesNotChangeTheTargetUsersPath(t *testing.T) { + ex, cli := pathInstallExecutor(t, "lmstudio", "command") + ex.addToPath = func(string, *pathReceipt, func() error) error { + t.Error("a remote install changed the local user's PATH") + return nil + } + s := &controlServer{exec: ex} + rec := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodPost, controlInstallPath, strings.NewReader(`{"opId":"path-test","engine":"lmstudio"}`)) + s.handleInstall(rec, req) + frames := decodeFrames(t, rec.Body.String()) + if len(frames) == 0 { + t.Fatal("missing install result") + } + last := frames[len(frames)-1] + if rec.Code != http.StatusOK { + t.Errorf("HTTP status = %d, want %d", rec.Code, http.StatusOK) + } + if last.Type != "result" { + t.Fatalf("terminal frame type = %q, want %q: %+v", last.Type, "result", last) + } + if last.Status == nil { + t.Fatal("the terminal result frame carried no status") + } + if !last.Status.Installed { + t.Error("the remote install reported the engine as not installed") + } + if !fileExists(cli) { + t.Fatal("remote install did not install the engine") + } + if last.Status.PathManaged { + t.Error("the result frame reported this user's PATH state to the peer") + } + // Only the installed flag, so a later consented local install can adopt it. + receipt, err := loadPathReceipt(ex.pathReceiptFile("lmstudio")) + if err != nil { + t.Fatal(err) + } + if !receipt.Installed || receipt.Dir != "" { + t.Fatalf("receipt after a remote install = %+v, want only the installed flag", receipt) + } +} + +func TestPeerEngineListHidesPathManaged(t *testing.T) { + ex, _ := pathInstallExecutor(t, "ollama", "process") + if err := savePathReceipt(ex.pathReceiptFile("ollama"), &pathReceipt{Dir: "d", WindowsEntry: "d"}); err != nil { + t.Fatal(err) + } + rec := httptest.NewRecorder() + (&controlServer{exec: ex}).handleEngines(rec, httptest.NewRequest(http.MethodGet, controlEnginesPath, nil)) + var body struct { + Engines []EngineStatus `json:"engines"` + } + if err := json.Unmarshal(rec.Body.Bytes(), &body); err != nil { + t.Fatal(err) + } + if len(body.Engines) == 0 { + t.Fatal("no engines served") + } + for _, e := range body.Engines { + if e.PathManaged { + t.Errorf("%s: served this user's PATH state to a peer", e.Engine) + } + } +} diff --git a/services/nvpair-engine-manager/lifecycle.go b/services/nvpair-engine-manager/lifecycle.go index cea7aa16..753f52f5 100644 --- a/services/nvpair-engine-manager/lifecycle.go +++ b/services/nvpair-engine-manager/lifecycle.go @@ -328,7 +328,11 @@ func (e *Executor) bringUpCommand(ctx context.Context, st *engineState, engine s argv := append([]string{launch.Bin}, launch.Args...) run := e.runCommand if rt.LaunchArgs != nil || rt.LaunchEnv != nil || len(launch.Env) > 0 { - run = func(ctx context.Context, argv []string) error { + // The variadic environment is runCommand's, for install commands + // that declare overrides. A launch carries its own in launch.Env, + // so the sole call below passes none and this ignores the parameter + // rather than pretending to merge two sources. + run = func(ctx context.Context, argv []string, _ ...string) error { return runPrivateLaunchCommand(ctx, argv, launch.Env, launchDiagnosticArgs(rt)) } // A vendor command may spawn its daemon and then exit unsuccessfully. diff --git a/services/nvpair-engine-manager/live_test.go b/services/nvpair-engine-manager/live_test.go index 6b84db8d..c8ac9bc2 100644 --- a/services/nvpair-engine-manager/live_test.go +++ b/services/nvpair-engine-manager/live_test.go @@ -128,7 +128,7 @@ func TestLiveOllamaCleanRoom(t *testing.T) { if r := waitResult(t, frames, "1", 5*time.Second); !strings.Contains(string(r), `"engine":"ollama"`) { t.Fatalf("ollama not listed: %s", r) } - send(t, stdin, 2, "engine:install", map[string]string{"engine": "ollama"}) + send(t, stdin, 2, "engine:install", map[string]any{"engine": "ollama", "path": true}) if r := waitResult(t, frames, "2", 600*time.Second); !strings.Contains(string(r), `"installed":true`) { t.Errorf("expected installed:true after install, got %s", r) } @@ -172,7 +172,7 @@ func TestLiveOllamaCleanRoom(t *testing.T) { } send(t, stdin, 5, "engine:stop", map[string]string{"engine": "ollama"}) waitResult(t, frames, "5", 30*time.Second) - send(t, stdin, 6, "engine:uninstall", map[string]string{"engine": "ollama"}) + send(t, stdin, 6, "engine:uninstall", map[string]any{"engine": "ollama", "path": true}) waitResult(t, frames, "6", 60*time.Second) send(t, stdin, 7, "shutdown", nil) waitResult(t, frames, "7", 5*time.Second) @@ -205,7 +205,7 @@ func TestLiveLMStudioCleanRoom(t *testing.T) { if r := waitResult(t, frames, "1", 5*time.Second); !strings.Contains(string(r), `"engine":"lmstudio"`) { t.Fatalf("lmstudio not listed: %s", r) } - send(t, stdin, 2, "engine:install", map[string]string{"engine": "lmstudio"}) + send(t, stdin, 2, "engine:install", map[string]any{"engine": "lmstudio", "path": true}) if r := waitResult(t, frames, "2", 600*time.Second); !strings.Contains(string(r), `"installed":true`) { t.Errorf("expected installed:true after install, got %s", r) } @@ -215,7 +215,7 @@ func TestLiveLMStudioCleanRoom(t *testing.T) { waitResult(t, frames, "4", 30*time.Second) send(t, stdin, 5, "engine:stop", map[string]string{"engine": "lmstudio"}) waitResult(t, frames, "5", 30*time.Second) - send(t, stdin, 6, "engine:uninstall", map[string]string{"engine": "lmstudio"}) + send(t, stdin, 6, "engine:uninstall", map[string]any{"engine": "lmstudio", "path": true}) waitResult(t, frames, "6", 60*time.Second) send(t, stdin, 7, "shutdown", nil) waitResult(t, frames, "7", 5*time.Second) @@ -223,10 +223,15 @@ func TestLiveLMStudioCleanRoom(t *testing.T) { // startManager spawns the manager binary with env overrides and returns // a frame stream, its stdin, and a cleanup func. It logs every frame. +// +// These tests install real engines, so PATH publishing is switched off: +// HKCU\Environment is machine state no temporary directory can contain, and a +// live run that fails partway must not leave the developer's account pointing at +// the temp directory it installed into. sandboxedEnv covers the dotfile half. func startManager(t *testing.T, env map[string]string) (chan frame, io.WriteCloser, func()) { t.Helper() - cmd := exec.Command(managerBin) - cmd.Env = overrideEnv(env) + cmd := exec.Command(managerBin, "--user-path=false") + cmd.Env = sandboxedEnv(t, env) stdin, err := cmd.StdinPipe() if err != nil { t.Fatal(err) @@ -270,20 +275,34 @@ func startManager(t *testing.T, env map[string]string) (chan frame, io.WriteClos return frames, stdin, cleanup } +// The vendor/product segments shared/appdir appends to the platform base +// directory, which is where the manager looks for override manifests. +const ( + appOrgDir = "Nvidia Corporation" + appProductDir = "Personal AI Router" +) + // startManagerWithManifest writes m into a temp config dir, then spawns // the manager pointed at it (so the override manifest shadows bundled). func startManagerWithManifest(t *testing.T, m Manifest) (chan frame, io.WriteCloser, func()) { t.Helper() - cfg := t.TempDir() - engdir := filepath.Join(cfg, configSubdir, "engines") - if err := os.MkdirAll(engdir, 0o755); err != nil { + cfg, home := t.TempDir(), t.TempDir() + data, err := json.MarshalIndent(m, "", " ") + if err != nil { t.Fatal(err) } - data, _ := json.MarshalIndent(m, "", " ") - if err := os.WriteFile(filepath.Join(engdir, m.Engine+".json"), data, 0o644); err != nil { - t.Fatal(err) + // appdir picks a different base per platform — %LocalAppData%, $XDG_CONFIG_HOME, + // or ~/Library/Application Support — so write to each rather than guess. + for _, base := range []string{cfg, filepath.Join(home, "Library", "Application Support")} { + engdir := filepath.Join(base, appOrgDir, appProductDir, "engines") + if err := os.MkdirAll(engdir, 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(engdir, m.Engine+".json"), data, 0o644); err != nil { + t.Fatal(err) + } } - return startManager(t, map[string]string{"APPDATA": cfg, "XDG_CONFIG_HOME": cfg}) + return startManager(t, map[string]string{"APPDATA": cfg, "LOCALAPPDATA": cfg, "XDG_CONFIG_HOME": cfg, "HOME": home}) } func sha256File(t *testing.T, path string) string { diff --git a/services/nvpair-engine-manager/main.go b/services/nvpair-engine-manager/main.go index 4a9b5765..fda77bdd 100644 --- a/services/nvpair-engine-manager/main.go +++ b/services/nvpair-engine-manager/main.go @@ -37,6 +37,11 @@ func main() { reservedPort := flag.Int("reserved-port", 0, "local engine port reserved by the parent proxy; 0 disables the reservation") clusterDir := flag.String("cluster-dir", "", "cluster identity/pin directory; when set and this node holds a cluster identity, the ec remote-control surface (--control-port) turns on with pin-based mTLS") loadedPollSec := flag.Int("loaded-poll-interval", defaultLoadedPollSeconds, "seconds between loaded-model polls that drive engine:models-changed pushes; 0 disables the watcher") + userPath := flag.Bool("user-path", true, "publish an installed engine's command-line directory on the current user's PATH; --user-path=false leaves HKCU\\Environment and the shell profiles alone") + // Not named removeUserPath: that is the package-level function this flag + // ultimately reaches, and shadowing it for the whole of main() sets a trap + // for the next person who needs the function here. + releaseUserPath := flag.Bool("remove-user-path", false, "release every PATH entry this user's engines own, then exit; the application uninstaller runs this before deleting the data directory that holds the ownership records") showVersion := flag.Bool("version", false, "print version and exit") resolveLevel := applog.RegisterFlag(nil, slog.LevelInfo) flag.Parse() @@ -48,6 +53,18 @@ func main() { applog.Init("nvpair-engine-manager", resolveLevel()) + if *releaseUserPath { + _, installBase := userPaths() + if installBase == "" { + log.Fatal("no user data directory: cannot locate the PATH ownership records") + } + if err := removeAllUserPaths(installBase); err != nil { + log.Fatalf("release user PATH entries: %v", err) + } + log.Print("released the PATH entries owned by this user's engines") + return + } + var transport io.ReadWriteCloser if *ipcPath != "" { conn, err := dialIPC(*ipcPath) @@ -90,6 +107,13 @@ func main() { // engine:set-port persists the chosen port as a manifest override in the // same per-user engines/ dir that buildRegistry overlays. exec.overrideDir = manifestDir + if !*userPath { + // The opt-in live tests drive a real install against a temporary home, + // so they must not reach the developer's registry value or dotfiles. + exec.addToPath = func(string, *pathReceipt, func() error) error { return nil } + exec.removeFromPath = func(*pathReceipt) error { return nil } + log.Print("user PATH updates are disabled (--user-path=false)") + } // Loaded-model watcher cadence. Integer seconds keeps parity with the other // flags, so the smallest positive interval is 1s; <=0 disables the watcher. if *loadedPollSec <= 0 { diff --git a/services/nvpair-engine-manager/manager.go b/services/nvpair-engine-manager/manager.go index e12f62dc..1c68aced 100644 --- a/services/nvpair-engine-manager/manager.go +++ b/services/nvpair-engine-manager/manager.go @@ -35,11 +35,17 @@ type engineParam struct { // opParam is the lifecycle-op input. Port (start) and Start (install) are // optional, per-call overrides; no manifest mutation, no persistence. +// +// Path is the user's answer about their PATH: on install, whether to publish the +// engine's CLI directory; on uninstall, whether to remove what PAIR published +// (false keeps the entries and hands them to the user). It is absent unless a +// client asked, and an absent answer changes neither PATH nor PAIR's claim. type opParam struct { Engine string `json:"engine"` Port int `json:"port,omitempty"` Bind string `json:"bind,omitempty"` Start bool `json:"start,omitempty"` + Path *bool `json:"path,omitempty"` } type actionParam struct { @@ -307,11 +313,15 @@ func (m *Manager) runOp(ctx context.Context, msg *Message) { var err error switch msg.Method { case "engine:install": - if err = m.exec.Install(ctx, p.Engine); err == nil && p.Start { + if err = m.exec.Install(ctx, p.Engine, p.Path != nil && *p.Path); err == nil && p.Start { err = start() } case "engine:uninstall": - err = m.exec.Uninstall(ctx, p.Engine) + if p.Path == nil { + err = m.exec.UninstallKeepingPathClaim(ctx, p.Engine) + } else { + err = m.exec.Uninstall(ctx, p.Engine, *p.Path) + } case "engine:start": err = start() case "engine:stop": diff --git a/services/nvpair-engine-manager/manifests/lmstudio.json b/services/nvpair-engine-manager/manifests/lmstudio.json index 964a7cc8..5b110621 100644 --- a/services/nvpair-engine-manager/manifests/lmstudio.json +++ b/services/nvpair-engine-manager/manifests/lmstudio.json @@ -5,6 +5,7 @@ "manifest_version": 1, "detect": ["~/.lmstudio/bin/lms"], "install": { + "env": { "LMS_NO_MODIFY_PATH": "1" }, "fetch": { "url": "https://lmstudio.ai/install.sh" }, "run": ["bash", "{download}"], "mode": "user" diff --git a/services/nvpair-engine-manager/pathlock_unix.go b/services/nvpair-engine-manager/pathlock_unix.go new file mode 100644 index 00000000..09a25e83 --- /dev/null +++ b/services/nvpair-engine-manager/pathlock_unix.go @@ -0,0 +1,28 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +//go:build !windows + +package main + +import ( + "errors" + "os" + "syscall" +) + +// tryLockExclusive takes the lock if it is free and reports false without +// waiting if another process holds it. The lock is released by unlockExclusive +// or by the process exiting, so a crash mid-update cannot wedge the next one — +// but a hung peer can, which is why the caller bounds its own wait. +func tryLockExclusive(f *os.File) (bool, error) { + err := syscall.Flock(int(f.Fd()), syscall.LOCK_EX|syscall.LOCK_NB) + if errors.Is(err, syscall.EWOULDBLOCK) { + return false, nil + } + return err == nil, err +} + +func unlockExclusive(f *os.File) error { + return syscall.Flock(int(f.Fd()), syscall.LOCK_UN) +} diff --git a/services/nvpair-engine-manager/pathlock_windows.go b/services/nvpair-engine-manager/pathlock_windows.go new file mode 100644 index 00000000..82fcf9d2 --- /dev/null +++ b/services/nvpair-engine-manager/pathlock_windows.go @@ -0,0 +1,32 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +package main + +import ( + "errors" + "os" + + "golang.org/x/sys/windows" +) + +// tryLockExclusive takes the lock if it is free and reports false without +// waiting if another process holds it. Windows releases a byte-range lock when +// the handle closes, including on an abnormal exit, so a crash mid-update +// cannot wedge the next one — but a hung peer can, which is why the caller +// bounds its own wait. +func tryLockExclusive(f *os.File) (bool, error) { + err := windows.LockFileEx( + windows.Handle(f.Fd()), + windows.LOCKFILE_EXCLUSIVE_LOCK|windows.LOCKFILE_FAIL_IMMEDIATELY, + 0, 1, 0, new(windows.Overlapped), + ) + if errors.Is(err, windows.ERROR_LOCK_VIOLATION) { + return false, nil + } + return err == nil, err +} + +func unlockExclusive(f *os.File) error { + return windows.UnlockFileEx(windows.Handle(f.Fd()), 0, 1, 0, new(windows.Overlapped)) +} diff --git a/services/nvpair-engine-manager/pathownership.go b/services/nvpair-engine-manager/pathownership.go new file mode 100644 index 00000000..b6f7d401 --- /dev/null +++ b/services/nvpair-engine-manager/pathownership.go @@ -0,0 +1,410 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +package main + +import ( + "encoding/json" + "errors" + "fmt" + "log/slog" + "os" + "path/filepath" + "strings" + "time" +) + +// A receipt is created only after PAIR installs an engine. PATH changes are +// recorded before writing them, so a failed write or process restart is retryable. +// Keep it outside the engine directory, which the vendor uninstaller deletes. +type pathReceipt struct { + // Dir is the directory PAIR published on the user's PATH. + Dir string `json:"dir,omitempty"` + WindowsEntry string `json:"windowsEntry,omitempty"` + ShellBlocks []pathBlock `json:"shellBlocks,omitempty"` + // Installed records that PAIR ran this engine's installer, which is a + // longer-lived fact than any individual PATH entry. It outlives the entries + // themselves: releasing PATH when the application is uninstalled keeps this + // flag, so a reinstall re-adopts an engine PAIR placed instead of mistaking + // it for one the user installed. Only an engine uninstall clears it. + // + // The directory is not a usable substitute. An engine whose vendor installer + // owns its location — LM Studio writes ~/.lmstudio — is indistinguishable + // from an external install by path alone. + Installed bool `json:"installed,omitempty"` +} + +// ownsEntries reports whether PAIR wrote a PATH entry this record can remove. +// A recorded Dir alone is not one: the directory may already have been on PATH, +// the login shell may be unsupported, or the write may have failed. +func (r *pathReceipt) ownsEntries() bool { + return r.WindowsEntry != "" || len(r.ShellBlocks) > 0 +} + +type pathBlock struct { + Profile string `json:"profile"` + Text string `json:"text"` +} + +// pathReceiptDir keeps ownership independent of the removable engine files. +func (e *Executor) pathReceiptDir() string { + return pathReceiptDir(e.baseDir) +} + +func pathReceiptDir(baseDir string) string { + return filepath.Join(baseDir, "engine-path") +} + +func (e *Executor) pathReceiptFile(engine string) string { + return filepath.Join(e.pathReceiptDir(), engine+".json") +} + +// errUnreadableReceipt marks a record that exists but cannot be parsed. +// +// The two directions need opposite things from it, so it is a distinct error +// rather than an empty record. Install must proceed: refusing would leave no +// way to clear the file short of the user finding and deleting it, and writing +// a fresh claim over it is both safe and self-repairing. Uninstall must not, +// because an empty record makes it skip the removal and then report the entries +// released — clearing the retry along with the warning, on evidence it never +// read. The entry outlives the only record that could identify it. +var errUnreadableReceipt = errors.New("PATH ownership record is unreadable") + +// loadPathReceipt returns an empty record for a file that is absent, and +// errUnreadableReceipt alongside one for a file that is present but corrupt. +// An absent record and an unreadable one are not the same fact, and callers +// that treat them alike report success they cannot back up. +func loadPathReceipt(file string) (*pathReceipt, error) { + data, err := os.ReadFile(file) + if errors.Is(err, os.ErrNotExist) { + return &pathReceipt{}, nil + } + if err != nil { + return nil, err + } + var receipt pathReceipt + if err := json.Unmarshal(data, &receipt); err != nil { + slog.Warn("PATH ownership record is unreadable", "file", file, "err", err) + return &pathReceipt{}, fmt.Errorf("%w: %s", errUnreadableReceipt, filepath.Base(file)) + } + return &receipt, nil +} + +// savePathReceipt atomically persists ownership before PATH is modified. +func savePathReceipt(file string, receipt *pathReceipt) error { + if err := os.MkdirAll(filepath.Dir(file), 0o700); err != nil { + return err + } + return writeJSONAtomic(file, receipt) +} + +// lockUserPath serializes a whole read-modify-write cycle — the receipt, the +// registry value or shell profiles, and the receipt again — within this process +// and across every process sharing the user's data directory. +// +// One mutex is not enough: nvpair-tui starts its own broker and its own +// engine-manager, so a terminal install can run concurrently with a desktop +// install against the same HKCU\Environment\Path, the same dotfiles, and the +// same receipts. Unsynchronized, the second write discards the first, and two +// shell installs can append duplicate blocks with only one removable record. +func lockUserPath(baseDir string) (func(), error) { + userPathMu.Lock() + release := func() { userPathMu.Unlock() } + dir := pathReceiptDir(baseDir) + if err := os.MkdirAll(dir, 0o700); err != nil { + release() + return nil, err + } + f, err := os.OpenFile(filepath.Join(dir, "lock"), os.O_CREATE|os.O_RDWR, 0o600) + if err != nil { + release() + return nil, err + } + if err := awaitExclusive(f, userPathLockTimeout); err != nil { + _ = f.Close() + release() + return nil, err + } + return func() { + _ = unlockExclusive(f) + _ = f.Close() + release() + }, nil +} + +// userPathLockTimeout bounds the wait for another process's PATH update. +// +// The engine's operation mutex is held across this wait, so waiting forever +// turns one wedged peer — a network home directory that stops responding, not +// a crash, which releases the lock — into every later install and uninstall +// for that engine hanging behind it. PATH work already degrades to a warning +// everywhere else, and the shell probes are bounded for the same reason. +const userPathLockTimeout = 10 * time.Second + +// awaitExclusive polls rather than blocking so the wait has an end. The timeout +// is a parameter rather than a package variable the test can reassign: the +// suite runs under -race, and a global swapped around a goroutine that reads it +// is a data race waiting for the one run where the timing lines up. +func awaitExclusive(f *os.File, timeout time.Duration) error { + for deadline := time.Now().Add(timeout); ; { + locked, err := tryLockExclusive(f) + if err != nil { + return err + } + if locked { + return nil + } + if time.Now().After(deadline) { + return fmt.Errorf("another process has held the PATH lock for over %s", timeout) + } + time.Sleep(50 * time.Millisecond) + } +} + +// pairOwnsInstall reports whether PAIR placed the engine already on disk, which +// is what entitles it to publish a PATH entry and later withdraw one. +// +// Any one of three facts establishes it: +// +// - a recorded directory, the ordinary case; +// - the Installed flag, which outlives the application uninstaller releasing +// every entry, so a reinstall re-adopts what PAIR placed; +// - an executable inside the engine's managed install directory, which covers +// a first install whose receipt never landed. +// +// The last is not sufficient on its own. An engine whose vendor installer owns +// its location never satisfies it, so testing only that made a lost record +// permanent for exactly the engine whose vendor PATH edit PAIR suppresses. +func pairOwnsInstall(receipt *pathReceipt, st *engineState) bool { + return receipt.Dir != "" || receipt.Installed || managedCLI(st) +} + +// updateInstallPath records fresh PAIR installs and retries only owned installs. +// It returns the directory it resolved so a failure can be logged against it. +func (e *Executor) updateInstallPath(engine string, st *engineState, installedNow bool) (string, error) { + unlock, err := lockUserPath(e.baseDir) + if err != nil { + return "", fmt.Errorf("lock PATH ownership: %w", err) + } + defer unlock() + file := e.pathReceiptFile(engine) + receipt, err := loadPathReceipt(file) + // A corrupt record is no claim for install's purposes: the fresh one written + // below replaces it, which is the only way the file ever gets repaired. + if err != nil && !errors.Is(err, errUnreadableReceipt) { + return "", fmt.Errorf("read PATH ownership: %w", err) + } + if !installedNow && !pairOwnsInstall(receipt, st) { + // An external installation is not ours to modify, and not ours to + // complain about either — the vendor's own installer handles its PATH. + return "", nil + } + // Resolved after the external check so a vendor layout PAIR cannot publish + // never produces a warning about a directory PAIR was never going to touch. + dir, err := pathCLIDir(st) + if err != nil { + return "", err + } + if receipt.Dir != "" && receipt.Dir != dir { + // The CLI moved — a manifest update, or a vendor that relocated it. + // Reaching here means PAIR owns the recorded entry, so release it before + // claiming the new one. Leaving it would point the user at a directory + // that no longer holds the engine, with the record still claiming PAIR + // put it there. + if err := e.removeFromPath(receipt); err != nil { + return dir, err + } + receipt = &pathReceipt{} + } + receipt.Dir = dir + receipt.Installed = true + save := func() error { return savePathReceipt(file, receipt) } + if err := save(); err != nil { + return dir, fmt.Errorf("save PATH ownership: %w", err) + } + return dir, e.addToPath(dir, receipt, save) +} + +// uninstallPath keeps the receipt on failure so cleanup can be retried even +// when the executable has already been removed by the vendor uninstaller. +func (e *Executor) uninstallPath(engine string) error { + unlock, lockErr := lockUserPath(e.baseDir) + if lockErr != nil { + return fmt.Errorf("lock PATH ownership: %w", lockErr) + } + defer unlock() + file := e.pathReceiptFile(engine) + // An unreadable record is a failure here, not an absent claim. Skipping the + // removal and reporting the entries released would strand them and clear the + // retry that is the only route back. + receipt, err := loadPathReceipt(file) + if err == nil && receipt.Dir != "" { + err = e.removeFromPath(receipt) + } + // The record goes even when it holds only the installed flag: the engine is + // uninstalled, and a flag left behind would make a later vendor install of + // the same engine look like one PAIR placed. + if err == nil { + if rmErr := os.Remove(file); rmErr != nil && !errors.Is(rmErr, os.ErrNotExist) { + err = rmErr + } + } + if err != nil { + err = fmt.Errorf("%s is uninstalled, but its PATH entries could not be cleaned up: %w", engine, err) + // redactHome for the same reason the install side does it: nvpair-errors + // push-syncs this message to every peer, and the wrapped error carries + // the absolute path of a file in this user's home directory. + e.reporter.report(serviceError{ID: uninstallFailedID(engine), Message: redactHome(err.Error()), Severity: "error", Action: "retry", EngineType: engine, Operation: "uninstall"}) + return err + } + e.reporter.clear(uninstallFailedID(engine)) + e.reporter.clear(pathFailedID(engine)) + return nil +} + +// recordInstalled notes that PAIR ran the engine's installer when the user +// declined the PATH entry. Nothing is published; the flag only keeps a later +// consented install able to re-adopt the engine. +// +// A claim already on record is kept, entries included. Declining is an answer +// to "may PAIR change PATH", so it changes nothing; withdrawing an entry takes +// an uninstall that asks. +func (e *Executor) recordInstalled(engine string) error { + unlock, err := lockUserPath(e.baseDir) + if err != nil { + return fmt.Errorf("lock PATH ownership: %w", err) + } + defer unlock() + file := e.pathReceiptFile(engine) + receipt, err := loadPathReceipt(file) + if err != nil && !errors.Is(err, errUnreadableReceipt) { + return fmt.Errorf("read PATH ownership: %w", err) + } + if receipt.Installed { + return nil + } + receipt.Installed = true + return savePathReceipt(file, receipt) +} + +// releasePathOwnership is the uninstall a user who kept the PATH entries asked +// for: the entries stay where they are and the record that claims them goes, so +// they are the user's from here on. +func (e *Executor) releasePathOwnership(engine string) error { + unlock, err := lockUserPath(e.baseDir) + if err != nil { + return fmt.Errorf("lock PATH ownership: %w", err) + } + defer unlock() + if err := os.Remove(e.pathReceiptFile(engine)); err != nil && !errors.Is(err, os.ErrNotExist) { + err = fmt.Errorf("%s is uninstalled, but its PATH ownership record could not be removed: %w", engine, err) + e.reporter.report(serviceError{ID: uninstallFailedID(engine), Message: redactHome(err.Error()), Severity: "error", Action: "retry", EngineType: engine, Operation: "uninstall"}) + return err + } + e.reporter.clear(uninstallFailedID(engine)) + e.reporter.clear(pathFailedID(engine)) + return nil +} + +// keepPathClaim is the uninstall a client made without asking the user. The +// engine is gone, so the installed flag goes with it, but PATH entries PAIR +// wrote stay claimed: dropping the claim on an unasked uninstall would leave +// entries nothing can identify, including the application uninstaller. +func (e *Executor) keepPathClaim(engine string) error { + unlock, err := lockUserPath(e.baseDir) + if err != nil { + return fmt.Errorf("lock PATH ownership: %w", err) + } + defer unlock() + file := e.pathReceiptFile(engine) + receipt, err := loadPathReceipt(file) + if errors.Is(err, errUnreadableReceipt) { + // Left as it is: it may be the only evidence of an entry, and a later + // uninstall that asks reports it. + return nil + } + if err != nil { + return fmt.Errorf("read PATH ownership: %w", err) + } + if receipt.ownsEntries() { + receipt.Installed = false + return savePathReceipt(file, receipt) + } + if err := os.Remove(file); err != nil && !errors.Is(err, os.ErrNotExist) { + return err + } + return nil +} + +// pathManaged reports whether PAIR currently owns a PATH entry for the engine, +// which is what decides whether an uninstall has anything to ask about. +// +// It reads the record on every call rather than caching it: nvpair-tui runs its +// own engine-manager against the same records, so a cached answer goes stale +// the moment the other one installs or uninstalls. The read takes no lock, +// because a status snapshot must not wait behind another process's PATH +// update; records are replaced by rename, so it sees the old or the new one. +func (e *Executor) pathManaged(engine string) bool { + receipt, err := loadPathReceipt(e.pathReceiptFile(engine)) + return err == nil && receipt.ownsEntries() +} + +// removeAllUserPaths releases every PATH entry the engines under baseDir own. +// +// The receipts live inside the data directory the application uninstaller +// deletes, and nothing in HKCU\Environment or the user's dotfiles is under it. +// Without this step, uninstalling PAIR — or uninstalling it without first +// uninstalling each engine — orphans those entries with no record left to +// remove them by. +func removeAllUserPaths(baseDir string) error { + return drainUserPaths(baseDir, removeUserPath) +} + +// drainUserPaths takes the remover as a parameter so the platform the tests run +// on does not decide which half of a receipt they can check. +func drainUserPaths(baseDir string, remove func(*pathReceipt) error) error { + unlock, err := lockUserPath(baseDir) + if err != nil { + return err + } + defer unlock() + dir := pathReceiptDir(baseDir) + entries, err := os.ReadDir(dir) + if errors.Is(err, os.ErrNotExist) { + return nil + } + if err != nil { + return err + } + var failures []error + for _, entry := range entries { + if entry.IsDir() || !strings.HasSuffix(entry.Name(), ".json") { + continue + } + file := filepath.Join(dir, entry.Name()) + receipt, err := loadPathReceipt(file) + if err != nil { + failures = append(failures, err) + continue + } + if err := remove(receipt); err != nil { + failures = append(failures, err) + continue + } + // The entries are gone; the installation is not. Retaining the Installed + // flag is what lets a later reinstall re-adopt an engine PAIR placed. + // Deleting the record outright made that unrecoverable for an engine + // whose CLI lives outside the managed install directory, because nothing + // else distinguishes it from one the user installed. + if receipt.Installed { + if err := savePathReceipt(file, &pathReceipt{Installed: true}); err != nil { + failures = append(failures, err) + } + continue + } + if err := os.Remove(file); err != nil { + failures = append(failures, err) + } + } + return errors.Join(failures...) +} diff --git a/services/nvpair-engine-manager/pathownership_test.go b/services/nvpair-engine-manager/pathownership_test.go new file mode 100644 index 00000000..5599772d --- /dev/null +++ b/services/nvpair-engine-manager/pathownership_test.go @@ -0,0 +1,668 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +package main + +import ( + "context" + "errors" + "os" + "path/filepath" + "strings" + "testing" + "time" +) + +func pathLifecycleExecutor(t *testing.T, engine, mode string) (*Executor, string, string) { + t.Helper() + ex, cli := pathInstallExecutor(t, engine, mode) + st := engineStateForTest(t, ex, engine) + // The uninstaller clears both the detected marker and the declared CLI, so + // the post-uninstall detect poll actually goes negative. + st.plat.Uninstall = &Uninstall{Run: append([]string{fakeEngineBin, "remove", cli}, st.plat.Detect...)} + platform := st.manifest.Platforms[hostKey()] + platform.Uninstall = st.plat.Uninstall + st.manifest.Platforms[hostKey()] = platform + home := t.TempDir() + ex.addToPath = func(dir string, receipt *pathReceipt, save func() error) error { + return addToShellPath(home, "sh", "", "", dir, receipt, save) + } + ex.removeFromPath = removeShellPath + return ex, cli, filepath.Join(home, ".profile") +} + +func TestUninstallRemovesOwnedPathAfterRestart(t *testing.T) { + test := func(engine, mode string) { + t.Run(engine, func(t *testing.T) { + ex, cli, profile := pathLifecycleExecutor(t, engine, mode) + original := "export PATH=\"$PATH:/user/tools\"\n" + if err := os.WriteFile(profile, []byte(original), 0o644); err != nil { + t.Fatal(err) + } + if err := ex.Install(context.Background(), engine, true); err != nil { + t.Fatal(err) + } + // A fresh executor must use the persisted ownership, not memory. + restarted := NewExecutor(ex.reg, NewReporter(nil), func(string, any) {}, ex.baseDir) + restarted.removeFromPath = removeShellPath + engineStateForTest(t, restarted, engine).installDir = engineStateForTest(t, ex, engine).installDir + if err := restarted.Uninstall(context.Background(), engine, true); err != nil { + t.Fatal(err) + } + data, err := os.ReadFile(profile) + if err != nil { + t.Fatal(err) + } + if string(data) != original { + t.Errorf("uninstall changed the user's profile to %q, want %q", data, original) + } + if fileExists(cli) { + t.Errorf("uninstall left the CLI behind at %q", cli) + } + if _, err := os.Stat(ex.pathReceiptFile(engine)); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("ownership receipt still exists: %v", err) + } + }) + } + test("ollama", "process") + test("lmstudio", "command") +} + +func TestDetectedExternalEngineDoesNotAcquirePathOwnership(t *testing.T) { + ex, cli, _ := pathLifecycleExecutor(t, "lmstudio", "command") + st := engineStateForTest(t, ex, "lmstudio") + // The vendor owns this location; PAIR never installed here. + st.installDir = t.TempDir() + for _, path := range append([]string{cli}, st.plat.Detect...) { + if err := os.WriteFile(path, []byte("external CLI"), 0o644); err != nil { + t.Fatal(err) + } + } + ex.addToPath = func(string, *pathReceipt, func() error) error { + t.Error("modified external engine PATH") + return nil + } + ex.removeFromPath = func(*pathReceipt) error { + t.Error("cleaned up an unowned PATH") + return nil + } + if err := ex.Install(context.Background(), "lmstudio", true); err != nil { + t.Fatal(err) + } + if err := ex.Uninstall(context.Background(), "lmstudio", true); err != nil { + t.Fatal(err) + } +} + +// A receipt that never landed must not make the next install report success +// while silently leaving PATH alone. +func TestManagedInstallWithNoReceiptStillClaimsPath(t *testing.T) { + ex, cli, _ := pathLifecycleExecutor(t, "ollama", "process") + if err := ex.Install(context.Background(), "ollama", true); err != nil { + t.Fatal(err) + } + if err := os.Remove(ex.pathReceiptFile("ollama")); err != nil { + t.Fatal(err) + } + var added string + ex.addToPath = func(dir string, _ *pathReceipt, _ func() error) error { added = dir; return nil } + // Already detected, so this install short-circuits before any download. + if err := ex.Install(context.Background(), "ollama", true); err != nil { + t.Fatal(err) + } + if added != filepath.Dir(cli) { + t.Fatalf("added %q, want the lost ownership to be re-recorded for %q", added, filepath.Dir(cli)) + } +} + +// A manifest update or a vendor relocation moves the CLI. The recorded entry +// still belongs to PAIR, so it has to move too rather than being left pointing +// at a directory the engine is no longer in. +func TestMovedCLIMigratesTheOwnedPathEntry(t *testing.T) { + ex, cli, profile := pathLifecycleExecutor(t, "ollama", "process") + if err := ex.Install(context.Background(), "ollama", true); err != nil { + t.Fatal(err) + } + before, err := os.ReadFile(profile) + if err != nil { + t.Fatal(err) + } + if !strings.Contains(string(before), filepath.Dir(cli)) { + t.Fatalf("install did not publish the original directory: %q", before) + } + + st := engineStateForTest(t, ex, "ollama") + moved := filepath.Join(st.installDir, "tools", filepath.Base(cli)) + if err := os.MkdirAll(filepath.Dir(moved), 0o755); err != nil { + t.Fatal(err) + } + if err := os.Rename(cli, moved); err != nil { + t.Fatal(err) + } + st.plat.Runtime.CLI = moved + + // Already detected, so this is the short-circuit path. + if err := ex.Install(context.Background(), "ollama", true); err != nil { + t.Fatal(err) + } + after, err := os.ReadFile(profile) + if err != nil { + t.Fatal(err) + } + if strings.Contains(string(after), filepath.Dir(cli)) { + t.Fatalf("stale entry survived the move: %q", after) + } + if !strings.Contains(string(after), filepath.Dir(moved)) { + t.Fatalf("new directory was not published: %q", after) + } + if warning := pathWarning(ex, "ollama"); warning != "" { + t.Fatalf("unexpected warning: %q", warning) + } +} + +// Wiping PAIR's data directory deletes the receipts but leaves the profile +// blocks, so a reinstall has to re-adopt a block it demonstrably authored. +func TestReinstallReadoptsItsOwnOrphanedProfileBlock(t *testing.T) { + ex, _, profile := pathLifecycleExecutor(t, "ollama", "process") + if err := ex.Install(context.Background(), "ollama", true); err != nil { + t.Fatal(err) + } + orphaned, err := os.ReadFile(profile) + if err != nil { + t.Fatal(err) + } + if len(orphaned) == 0 { + t.Fatal("install wrote no PATH block") + } + // Simulate the data wipe: the receipts are inside the deleted tree, the + // shell profile is not. + if err := os.RemoveAll(ex.pathReceiptDir()); err != nil { + t.Fatal(err) + } + if err := ex.Install(context.Background(), "ollama", true); err != nil { + t.Fatal(err) + } + after, err := os.ReadFile(profile) + if err != nil { + t.Fatal(err) + } + if string(after) != string(orphaned) { + t.Fatalf("reinstall duplicated the block: %q", after) + } + if err := ex.Uninstall(context.Background(), "ollama", true); err != nil { + t.Fatal(err) + } + remaining, err := os.ReadFile(profile) + if err != nil { + t.Fatal(err) + } + if len(remaining) != 0 { + t.Fatalf("uninstall left the re-adopted block behind: %q", remaining) + } +} + +// The application uninstaller runs this before deleting the data directory the +// receipts live in. +func TestDrainReleasesThePathBlock(t *testing.T) { + ex, _, profile := pathLifecycleExecutor(t, "ollama", "process") + if err := ex.Install(context.Background(), "ollama", true); err != nil { + t.Fatal(err) + } + contents, err := os.ReadFile(profile) + if err != nil { + t.Fatal(err) + } + if len(contents) == 0 { + t.Fatal("install wrote no PATH block, so the drain would prove nothing") + } + if err := drainUserPaths(ex.baseDir, removeShellPath); err != nil { + t.Fatal(err) + } + after, err := os.ReadFile(profile) + if err != nil { + t.Fatal(err) + } + if len(after) != 0 { + t.Fatalf("drain left a PATH block: %q", after) + } +} + +// Draining releases the entries but not the knowledge that PAIR installed the +// engine. Deleting the record outright is what made a reinstall unable to +// republish PATH for an engine whose CLI lives outside PAIR's install +// directory, because nothing else distinguishes it from an external install. +func TestDrainKeepsTheInstalledFlagSoReinstallCanReadopt(t *testing.T) { + ex, _, _ := pathLifecycleExecutor(t, "ollama", "process") + if err := ex.Install(context.Background(), "ollama", true); err != nil { + t.Fatal(err) + } + if err := drainUserPaths(ex.baseDir, removeShellPath); err != nil { + t.Fatal(err) + } + receipt, err := loadPathReceipt(ex.pathReceiptFile("ollama")) + if err != nil { + t.Fatal(err) + } + if !receipt.Installed { + t.Error("drain dropped the installed flag, so a reinstall cannot re-adopt the engine") + } + if receipt.Dir != "" { + t.Errorf("drain kept a PATH claim it had already released: %q", receipt.Dir) + } + if len(receipt.ShellBlocks) != 0 { + t.Errorf("drain kept %d shell block claims it had already released", len(receipt.ShellBlocks)) + } +} + +// Draining an engine PAIR never installed leaves nothing behind at all. +func TestDrainRemovesARecordWithNoInstallBehindIt(t *testing.T) { + ex, _, _ := pathLifecycleExecutor(t, "ollama", "process") + file := ex.pathReceiptFile("ollama") + if err := savePathReceipt(file, &pathReceipt{Dir: t.TempDir()}); err != nil { + t.Fatal(err) + } + if err := drainUserPaths(ex.baseDir, removeShellPath); err != nil { + t.Fatal(err) + } + if _, err := os.Stat(file); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("drain kept a record with no installation behind it: %v", err) + } +} + +// LM Studio installs into ~/.lmstudio, outside the directory PAIR installs +// into, so an executable-location test can never recognize it as PAIR's. The +// record has to carry that fact instead: the application uninstaller releases +// every PATH entry while leaving the engine installed, and without the retained +// flag a reinstall reads the engine as an external install and never republishes +// its CLI — permanently, because PAIR also suppresses the vendor's own PATH edit. +func TestReinstallRepublishesPathForAnEngineOutsideTheInstallDirectory(t *testing.T) { + ex, cli, _ := pathLifecycleExecutor(t, "lmstudio", "command") + engineStateForTest(t, ex, "lmstudio").installDir = t.TempDir() + if err := ex.Install(context.Background(), "lmstudio", true); err != nil { + t.Fatal(err) + } + // The application uninstaller: release the entries, leave the engine. + if err := drainUserPaths(ex.baseDir, removeShellPath); err != nil { + t.Fatal(err) + } + var added string + ex.addToPath = func(dir string, _ *pathReceipt, _ func() error) error { added = dir; return nil } + if err := ex.Install(context.Background(), "lmstudio", true); err != nil { + t.Fatal(err) + } + if added != filepath.Dir(cli) { + t.Fatalf("reinstall published %q, want the engine's CLI directory %q", added, filepath.Dir(cli)) + } +} + +// Declining to touch an external install is not the same as succeeding at it. +// Clearing the warning there retracts a standing report about a CLI that is +// still missing from the user's PATH. +func TestSkippingAnExternalInstallLeavesTheWarningStanding(t *testing.T) { + ex, _, _ := pathLifecycleExecutor(t, "lmstudio", "command") + st := engineStateForTest(t, ex, "lmstudio") + st.installDir = t.TempDir() + for _, path := range append([]string{}, st.plat.Detect...) { + if err := os.WriteFile(path, []byte("external CLI"), 0o644); err != nil { + t.Fatal(err) + } + } + ex.reporter.report(serviceError{ID: pathFailedID("lmstudio"), Message: "an earlier attempt failed", Severity: "warning"}) + if err := ex.Install(context.Background(), "lmstudio", true); err != nil { + t.Fatal(err) + } + if pathWarning(ex, "lmstudio") == "" { + t.Error("a skipped external install cleared a warning it never addressed") + } +} + +// A record PAIR cannot parse is not a record of no claim. Reporting the entries +// released would strand them and clear the retry that is the only route back. +func TestUninstallWithAnUnreadableReceiptReportsFailure(t *testing.T) { + ex, _, _ := pathLifecycleExecutor(t, "ollama", "process") + if err := ex.Install(context.Background(), "ollama", true); err != nil { + t.Fatal(err) + } + file := ex.pathReceiptFile("ollama") + if err := os.WriteFile(file, []byte("{ this is not json"), 0o600); err != nil { + t.Fatal(err) + } + err := ex.Uninstall(context.Background(), "ollama", true) + if !errors.Is(err, errUnreadableReceipt) { + t.Fatalf("uninstall returned %v, want the unreadable record to surface", err) + } + if _, statErr := os.Stat(file); statErr != nil { + t.Errorf("uninstall deleted the record it could not read: %v", statErr) + } +} + +// The lock file is what carries this across processes: nvpair-tui starts its own +// broker and its own engine-manager against the same data directory, and the +// application uninstaller drains PATH from a third. Two independent handles +// stand in for those separate processes — an in-process mutex alone would let +// the second update overwrite the first. +func TestPathLockIsExclusiveAcrossHandles(t *testing.T) { + file := filepath.Join(t.TempDir(), "lock") + open := func() *os.File { + t.Helper() + f, err := os.OpenFile(file, os.O_CREATE|os.O_RDWR, 0o600) + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = f.Close() }) + return f + } + held, other := open(), open() + + locked, err := tryLockExclusive(held) + if err != nil { + t.Fatal(err) + } + if !locked { + t.Fatal("could not take a lock nothing else held") + } + locked, err = tryLockExclusive(other) + if err != nil { + t.Fatal(err) + } + if locked { + t.Error("a second handle took a lock the first already held") + } + if err := unlockExclusive(held); err != nil { + t.Fatal(err) + } + locked, err = tryLockExclusive(other) + if err != nil { + t.Fatal(err) + } + if !locked { + t.Error("the lock stayed held after being released") + } +} + +// The engine's operation mutex is held across this wait, so a peer that hangs +// rather than crashing must not park every later install behind it. +func TestPathLockWaitIsBounded(t *testing.T) { + baseDir := t.TempDir() + unlock, err := lockUserPath(baseDir) + if err != nil { + t.Fatal(err) + } + defer unlock() + + // A separate handle stands in for the second process; the in-process mutex + // is already held by this goroutine's lock, so the wait under test is the + // file lock's. + waited := make(chan error, 1) + go func() { + f, openErr := os.OpenFile(filepath.Join(pathReceiptDir(baseDir), "lock"), os.O_CREATE|os.O_RDWR, 0o600) + if openErr != nil { + waited <- openErr + return + } + defer f.Close() + waited <- awaitExclusive(f, 150*time.Millisecond) + }() + select { + case err := <-waited: + if err == nil { + t.Fatal("took a lock another handle held") + } + case <-time.After(5 * time.Second): + t.Fatal("the wait for a held lock never ended") + } +} + +// A corrupt receipt must not wedge install for that engine: the fresh claim +// written over it is the only way the file is ever repaired. +func TestUnreadableReceiptIsTreatedAsNoClaim(t *testing.T) { + ex, cli, _ := pathLifecycleExecutor(t, "ollama", "process") + if err := ex.Install(context.Background(), "ollama", true); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(ex.pathReceiptFile("ollama"), []byte("{ truncated"), 0o600); err != nil { + t.Fatal(err) + } + var added string + ex.addToPath = func(dir string, _ *pathReceipt, _ func() error) error { added = dir; return nil } + if err := ex.Install(context.Background(), "ollama", true); err != nil { + t.Fatalf("install refused to proceed past a corrupt receipt: %v", err) + } + if added != filepath.Dir(cli) { + t.Fatalf("added %q, want %q", added, filepath.Dir(cli)) + } + if err := ex.Uninstall(context.Background(), "ollama", true); err != nil { + t.Fatalf("uninstall refused to proceed past a corrupt receipt: %v", err) + } +} + +func TestFailedUninstallPreservesPathOwnership(t *testing.T) { + ex, _, profile := pathLifecycleExecutor(t, "ollama", "process") + if err := ex.Install(context.Background(), "ollama", true); err != nil { + t.Fatal(err) + } + before, err := os.ReadFile(profile) + if err != nil { + t.Fatal(err) + } + st, err := ex.state("ollama") + if err != nil { + t.Fatal(err) + } + // The command returns successfully but leaves the executable in place. + st.plat.Uninstall.Run = []string{fakeEngineBin, "echo", "still installed"} + ex.detectTimeout = time.Millisecond + if err := ex.Uninstall(context.Background(), "ollama", true); err == nil { + t.Fatal("expected failed uninstall") + } + after, err := os.ReadFile(profile) + if err != nil { + t.Fatal(err) + } + if string(after) != string(before) { + t.Fatal("failed uninstall changed PATH") + } + if _, err := os.Stat(ex.pathReceiptFile("ollama")); err != nil { + t.Fatalf("lost ownership after failed uninstall: %v", err) + } +} + +func TestPathCleanupCanRetryAfterEngineRemoval(t *testing.T) { + ex, cli, profile := pathLifecycleExecutor(t, "lmstudio", "command") + if err := ex.Install(context.Background(), "lmstudio", true); err != nil { + t.Fatal(err) + } + wantErr := errors.New("profile is read only") + ex.removeFromPath = func(*pathReceipt) error { return wantErr } + if err := ex.Uninstall(context.Background(), "lmstudio", true); !errors.Is(err, wantErr) { + t.Fatalf("uninstall error = %v", err) + } + if fileExists(cli) { + t.Fatal("engine was not removed before cleanup") + } + ex.removeFromPath = removeShellPath + if err := ex.Uninstall(context.Background(), "lmstudio", true); err != nil { + t.Fatal(err) + } + contents, err := os.ReadFile(profile) + if err != nil { + t.Fatal(err) + } + if len(contents) != 0 { + t.Fatalf("retry left PATH block: %q", contents) + } +} + +// The published PATH and the recorded claim are asserted directly, not just the +// round trip through removal. A no-op addWindowsPath leaves next unchanged and +// the receipt empty, and removeWindowsPath returns an unclaimed PATH untouched — +// so every round trip here would still match its expectation with the append +// deleted entirely. +func TestWindowsPathOwnershipPreservesExistingEntries(t *testing.T) { + test := func(name, current, wantPublished, wantClaim, wantAfterCleanup string) { + t.Run(name, func(t *testing.T) { + receipt := &pathReceipt{} + expand := func(s string) string { return strings.ReplaceAll(s, "%ENGINE%", `C:\Engine`) } + next, err := addWindowsPath(current, `C:\Engine`, expand, receipt, func() error { return nil }) + if err != nil { + t.Fatal(err) + } + if next != wantPublished { + t.Errorf("published PATH = %q, want %q", next, wantPublished) + } + if receipt.WindowsEntry != wantClaim { + t.Errorf("recorded claim = %q, want %q", receipt.WindowsEntry, wantClaim) + } + if got := removeWindowsPath(next, receipt.WindowsEntry); got != wantAfterCleanup { + t.Errorf("after cleanup = %q, want %q", got, wantAfterCleanup) + } + }) + } + test("new entry", `C:\UserTools`, `C:\UserTools;C:\Engine`, `C:\Engine`, `C:\UserTools`) + // Appending after a trailing separator would turn a harmless trailing empty + // element into a searched interior one, so the separator is dropped instead. + test("trailing empty entry", `C:\UserTools;`, `C:\UserTools;C:\Engine`, `C:\Engine`, `C:\UserTools`) + // Already on PATH: nothing is appended, and nothing is claimed. Recording an + // entry the user put there would let uninstall delete it. + test("existing entry", `C:\Engine;C:\UserTools`, `C:\Engine;C:\UserTools`, "", `C:\Engine;C:\UserTools`) + test("existing case variant", `c:\ENGINE;C:\UserTools`, `c:\ENGINE;C:\UserTools`, "", `c:\ENGINE;C:\UserTools`) + test("existing environment reference", `%ENGINE%;C:\UserTools`, `%ENGINE%;C:\UserTools`, "", `%ENGINE%;C:\UserTools`) +} + +// Addition compares case-insensitively after normalizing, so removal has to as +// well: otherwise a PATH editor that re-cased or re-slashed the entry would make +// it unremovable, and uninstall would silently leave it pointing at a deleted +// directory. +func TestWindowsCleanupMatchesRespelledEntries(t *testing.T) { + test := func(name, current, want string) { + t.Run(name, func(t *testing.T) { + if got := removeWindowsPath(current, `C:\Engine`); got != want { + t.Fatalf("after cleanup = %q, want %q", got, want) + } + }) + } + test("recased", `C:\UserTools;c:\engine`, `C:\UserTools`) + test("trailing backslash", `C:\UserTools;C:\Engine\`, `C:\UserTools`) + test("quoted", `"C:\Engine";C:\UserTools`, `C:\UserTools`) + test("forward slashes", `C:/Engine;C:\UserTools`, `C:\UserTools`) +} + +// PAIR appends, and an installer that wants precedence prepends, so a duplicate +// is somebody else's copy in front of PAIR's. Take PAIR's and leave theirs +// where they put it, rather than silently demoting a directory another tool +// deliberately promoted. +func TestWindowsCleanupRemovesTheAppendedDuplicate(t *testing.T) { + current := `C:\Engine;C:\UserTools;C:\Engine` + if got, want := removeWindowsPath(current, `C:\Engine`), `C:\Engine;C:\UserTools`; got != want { + t.Fatalf("after cleanup = %q, want %q", got, want) + } +} + +func TestWindowsPathOwnershipSavedBeforeChangingPath(t *testing.T) { + wantErr := errors.New("cannot save receipt") + current := `C:\UserTools` + got, err := addWindowsPath(current, `C:\Engine`, func(s string) string { return s }, &pathReceipt{}, func() error { return wantErr }) + if !errors.Is(err, wantErr) || got != current { + t.Fatalf("PATH = %q, error = %v", got, err) + } +} + +func TestWindowsCleanupPreservesUserModifiedEntry(t *testing.T) { + current := `C:\UserTools;%ENGINE%` + if got := removeWindowsPath(current, `C:\Engine`); got != current { + t.Fatalf("modified user entry was removed: %q", got) + } +} + +func TestShellOwnershipSaveFailureLeavesProfileUntouched(t *testing.T) { + home := t.TempDir() + profile := filepath.Join(home, ".profile") + const original = "# user configuration\n" + if err := os.WriteFile(profile, []byte(original), 0o644); err != nil { + t.Fatal(err) + } + wantErr := errors.New("cannot save ownership") + err := addToShellPath(home, "sh", "", "", "/engine/bin", &pathReceipt{}, func() error { return wantErr }) + if !errors.Is(err, wantErr) { + t.Fatalf("error = %v", err) + } + contents, err := os.ReadFile(profile) + if err != nil { + t.Fatal(err) + } + if string(contents) != original { + t.Fatalf("profile changed before ownership was saved: %q", contents) + } +} + +// Once a user edits the block, PAIR can no longer prove it wrote what is there, +// so cleanup leaves it alone rather than guessing at the boundaries. +func TestShellCleanupPreservesAUserEditedBlock(t *testing.T) { + home := t.TempDir() + profile := filepath.Join(home, ".profile") + if err := os.WriteFile(profile, []byte("export PATH=\"$PATH:/engine/bin\"\n"), 0o644); err != nil { + t.Fatal(err) + } + receipt := &pathReceipt{} + if err := addToShellPath(home, "sh", "", "", "/engine/bin", receipt, func() error { return nil }); err != nil { + t.Fatal(err) + } + before, err := os.ReadFile(profile) + if err != nil { + t.Fatal(err) + } + before = []byte(strings.ReplaceAll(string(before), "PAIR engine", "user customized engine")) + if err := os.WriteFile(profile, before, 0o644); err != nil { + t.Fatal(err) + } + if err := removeShellPath(receipt); err != nil { + t.Fatal(err) + } + after, err := os.ReadFile(profile) + if err != nil { + t.Fatal(err) + } + if string(after) != string(before) { + t.Fatal("cleanup modified unowned profile content") + } +} + +// Finding the exact block already present means an earlier PAIR install wrote it +// and lost the receipt. Recording ownership anyway is what lets the next +// uninstall clean it up instead of orphaning it forever. +func TestShellAddAdoptsAnIdenticalBlockItDidNotRecord(t *testing.T) { + home := t.TempDir() + profile := filepath.Join(home, ".profile") + const userLine = "export PATH=\"$PATH:/user/tools\"\n" + if err := os.WriteFile(profile, []byte(userLine), 0o644); err != nil { + t.Fatal(err) + } + if err := testAddToShellPath(home, "sh", "", "", "/engine/bin"); err != nil { + t.Fatal(err) + } + orphaned, err := os.ReadFile(profile) + if err != nil { + t.Fatal(err) + } + receipt := &pathReceipt{} + if err := addToShellPath(home, "sh", "", "", "/engine/bin", receipt, func() error { return nil }); err != nil { + t.Fatal(err) + } + readopted, err := os.ReadFile(profile) + if err != nil { + t.Fatal(err) + } + if string(readopted) != string(orphaned) { + t.Fatalf("adoption duplicated the block: %q", readopted) + } + if err := removeShellPath(receipt); err != nil { + t.Fatal(err) + } + after, err := os.ReadFile(profile) + if err != nil { + t.Fatal(err) + } + if string(after) != userLine { + t.Fatalf("after cleanup = %q, want only the user's own line", after) + } +} diff --git a/services/nvpair-engine-manager/registry.go b/services/nvpair-engine-manager/registry.go index b6a79ad6..5d7b7a94 100644 --- a/services/nvpair-engine-manager/registry.go +++ b/services/nvpair-engine-manager/registry.go @@ -82,6 +82,32 @@ type Install struct { // an explicit, rarely-used exception the runner surfaces loudly // rather than silently escalating. Mode string `json:"mode,omitempty"` + // Env holds literal environment overrides for the installer subprocess + // only — not the manager's environment and not the engine's runtime. It + // keeps a vendor quirk (LM Studio's installer edits PATH unless + // LMS_NO_MODIFY_PATH is set) in the manifest, so the runner stays + // engine-agnostic and a third-party engine can declare the same thing + // without a code change. + Env map[string]string `json:"env,omitempty"` +} + +// environ renders Env as the KEY=VALUE overrides runCommand layers onto the +// inherited environment. Deterministically ordered so a failing install command +// reproduces identically. +func (i *Install) environ() []string { + if len(i.Env) == 0 { + return nil + } + keys := make([]string, 0, len(i.Env)) + for k := range i.Env { + keys = append(keys, k) + } + sort.Strings(keys) + out := make([]string, 0, len(keys)) + for _, k := range keys { + out = append(out, k+"="+i.Env[k]) + } + return out } // Uninstall removes a user-mode install by running the engine's own diff --git a/services/nvpair-engine-manager/remediation_test.go b/services/nvpair-engine-manager/remediation_test.go index 1999373b..18712829 100644 --- a/services/nvpair-engine-manager/remediation_test.go +++ b/services/nvpair-engine-manager/remediation_test.go @@ -116,7 +116,7 @@ func TestUninstallRetries(t *testing.T) { m.Platforms[key] = p ex := newTestExecutor(t, m) - err := ex.Uninstall(context.Background(), "fake") + err := ex.Uninstall(context.Background(), "fake", true) if err == nil || !strings.Contains(err.Error(), "after 3 attempts") { t.Fatalf("expected uninstall failure after 3 attempts, got %v", err) } @@ -284,7 +284,7 @@ func TestInstallChecksumMismatchReported(t *testing.T) { }}, } ex, c := capturingExecutor(t, m) - err := ex.Install(context.Background(), "fake") + err := ex.Install(context.Background(), "fake", true) if err == nil || !strings.Contains(err.Error(), "checksum mismatch") { t.Fatalf("expected checksum mismatch, got %v", err) } diff --git a/services/nvpair-engine-manager/setport.go b/services/nvpair-engine-manager/setport.go index 66f0b283..9f342e5c 100644 --- a/services/nvpair-engine-manager/setport.go +++ b/services/nvpair-engine-manager/setport.go @@ -218,28 +218,36 @@ func setOverrideObject(parent map[string]any, key string, object map[string]any) } // writeJSONAtomic marshals v and writes it to path via a tmp file + rename so -// a crash mid-write can't leave a truncated manifest behind. +// a crash mid-write can't leave a truncated manifest behind. The temporary file +// is flushed before the rename and the directory after it: rename makes the +// swap atomic for a concurrent reader, but on its own guarantees nothing about +// the bytes reaching stable storage, so a power loss could publish an empty file. func writeJSONAtomic(path string, v any) error { data, err := json.MarshalIndent(v, "", " ") if err != nil { return err } - file, err := os.CreateTemp(filepath.Dir(path), ".override-*.tmp") + // Named by file, not by caller: this also persists PATH ownership records, + // and a failed receipt write reported as "write override" sends the reader + // looking at port overrides. + name := filepath.Base(path) + file, err := os.CreateTemp(filepath.Dir(path), "."+name+"-*.tmp") if err != nil { - return fmt.Errorf("create override: %w", err) + return fmt.Errorf("write %s: %w", name, err) } tmp := file.Name() defer os.Remove(tmp) - if _, err := file.Write(data); err != nil { - _ = file.Close() - return fmt.Errorf("write override: %w", err) - } - if err := file.Close(); err != nil { - return fmt.Errorf("close override: %w", err) - } - if err := os.Rename(tmp, path); err != nil { - _ = os.Remove(tmp) - return fmt.Errorf("rename override: %w", err) - } - return nil + // Closes the file on every path. 0600 is asserted by + // TestSettingsOverrideRestrictsExistingPermissions and is stated rather + // than inherited from CreateTemp, because the rename has to *restrict* a + // pre-existing group- or world-readable file, not merely avoid widening it. + // Every caller writes a per-user file: a launch environment that can hold + // credentials, a desired-state record, or a PATH ownership receipt. + if err := writeAndSync(file, data, 0o600); err != nil { + return fmt.Errorf("write %s: %w", name, err) + } + if err := replaceFile(tmp, path); err != nil { + return fmt.Errorf("rename %s: %w", name, err) + } + return syncDir(filepath.Dir(path)) } diff --git a/services/nvpair-engine-manager/spec.md b/services/nvpair-engine-manager/spec.md index ba2e6324..25154b74 100644 --- a/services/nvpair-engine-manager/spec.md +++ b/services/nvpair-engine-manager/spec.md @@ -33,11 +33,15 @@ A declarative, config-driven control plane for **local inference engines** (Olla - **The node's error list** — owned by `nvpair-errors`, which holds it as in-memory session state; this service only emits `errors:report` / `errors:clear`. ## 3. Key Use Cases -- **Install an engine, user-mode**: `engine:install {engine:"ollama"}` downloads the per-OS user-scoped package (Windows/Linux standalone archive extracted into a user dir; macOS app bundle — never an elevated `Setup.exe` or `curl | sh`), checksum-verifies, extracts, re-detects. +- **Install an engine, user-mode**: `engine:install {engine:"ollama"}` downloads the per-OS user-scoped package (Windows/Linux standalone archive extracted into a user dir; macOS app bundle — never an elevated `Setup.exe` or `curl | sh`), checksum-verifies, extracts, re-detects, then — when the request carries `path:true` — publishes the engine's CLI directory on this user's PATH. +- **PATH consent**: every PATH change is the user's decision, carried as `path` on `engine:install` and `engine:uninstall`. A missing or false `path` never touches PATH. A declined install still records that PAIR ran the installer, and keeps any claim already on record; so does a peer's remote install, which never publishes. A declined uninstall (`path:false`) leaves the entries and deletes the record, handing them to the user. An uninstall with no `path` means the client did not ask: the entries stay and so does PAIR's claim on them, so an unasked uninstall can never strand an entry. `EngineStatus.path_managed` is true only while the record holds an entry PAIR wrote — a Windows entry or a shell block — and is emitted after every PATH step; the control surface serves it to peers as false. +- **Publish the CLI on PATH**: the directory comes from `runtime.cli`, or from the detected executable when the manifest declares none and that executable is inside the directory PAIR installed into. Ownership is recorded before the change, under a cross-process lock, so uninstall removes exactly what PAIR added. A remote install from a cluster peer (`InstallForPeer`) skips this step: editing the login shell of whoever is sitting at the target node is not something a paired peer decides. - **Run lifecycle**: `engine:start` / `engine:stop` / `engine:restart` / `engine:status`, with readiness and health probes against the engine's loopback port. - **Run a declared action**: `engine:action {engine, action, params}` → the manifest-declared HTTP call to the engine's loopback control API (e.g. `127.0.0.1:{port}/api/pull`). (Methods, notifications, and UI events all use the colon form `engine:*`, matching the POC UI and the `errors:*` notifications.) - **Onboard a new engine (no code)**: a vendor adds `engines/.json`; the generic runner exposes their lifecycle + actions immediately. -- **Edge case — already installed**: detect short-circuits install (idempotent). +- **Edge case — already installed**: detect short-circuits the download and the install command. With `path:true` the PATH step still runs, so an engine whose ownership record was lost — the data directory was wiped, or an earlier save failed — reacquires it instead of reporting success with nothing on PATH. Recovery does not depend on where the CLI lives: the record's `installed` flag survives the application uninstaller's release, and a wiped record is recovered from the executable's location only when that location is inside PAIR's install directory. It is idempotent: an entry PAIR already owns is left as it is. If the CLI has moved — a manifest update, or a vendor that relocated it — the recorded entry is released and the new directory claimed, so the stale one does not outlive the engine it pointed at. +- **Edge case — PATH setup fails**: reported as a dismissible warning, never an install failure. The engine is installed and usable through its full path, and the caller's optional start step still runs. +- **Edge case — engine already present before PAIR**: no PATH entry and no warning. Its own installer owns that location. - **Edge case — checksum mismatch**: install fails before `run`, is reported, and never executes an unverified payload. - **Edge case — readiness timeout / crash**: failed readiness returns to Stopped; a later crash flips health and is reported. @@ -77,6 +81,7 @@ A declarative, config-driven control plane for **local inference engines** (Olla running: boolean healthy: boolean port: number + path_managed: boolean // PAIR owns a PATH entry for this engine on this machine } ``` @@ -112,8 +117,8 @@ Requests (caller → service): | `engine:get-installed` | — | `{ engines: [EngineStatus] }` | | `engine:describe` | `{ engine }` | the engine's manifest | | `engine:status` | `{ engine }` | `EngineStatus` | -| `engine:install` | `{ engine }` | `EngineStatus` (after install) | -| `engine:uninstall` | `{ engine }` | `EngineStatus` (after removal) | +| `engine:install` | `{ engine, start?, path? }` | `EngineStatus` (after install, and PATH publication when `path:true`) | +| `engine:uninstall` | `{ engine, path? }` | `EngineStatus` (after removal, and PATH cleanup when `path:true`) | | `engine:start` | `{ engine }` | `EngineStatus` (after readiness) | | `engine:stop` | `{ engine }` | `EngineStatus` | | `engine:restart` | `{ engine }` | `EngineStatus` | @@ -158,14 +163,15 @@ The `engine:remote-*` methods are the client half: engine-manager resolves the t - **External**: engine vendors' download URLs (HTTPS; checksum-pinned when a `sha256` is set, else HTTPS-only with a warning); first-party `nvpair-shared/applog` (logging + `log/set-level`) and `nvpair-shared/clustertrust` (pin-based mTLS). The `nvpair-shared/errors` wire shape is mirrored locally with identical JSON tags. No third-party runtime services. ## 9. Data Ownership -- **Owned**: the in-memory engine registry (parsed manifests + per-engine runtime state) and per-engine log/error ring buffers — transient only. -- **Source of truth**: no — `nvpair-errors` owns the node's error list (in memory, for the session); model inventories belong to the engines; manifests on disk are authored elsewhere. -- **Storage**: in-memory; manifests read from the per-user data dir's `engines/*.json` (`%LocalAppData%\Nvidia Corporation\Personal AI Router` on Windows, `~/.config/Nvidia Corporation/Personal AI Router` on Linux, `~/Library/Application Support/Nvidia Corporation/Personal AI Router` on macOS) plus bundled `manifests/*.json`. No database. +- **Owned**: the in-memory engine registry (parsed manifests + per-engine runtime state) and per-engine log/error ring buffers — transient. Also the durable PATH ownership records under the per-user data dir's `engine-bin/engine-path/.json`, which are **not** transient: they are the only thing that can identify what PAIR added to the user's PATH, and they have to outlive both a restart and the engine's own files. +- **Source of truth**: yes, for what PAIR published on this user's PATH — nothing else records it. Otherwise no: `nvpair-errors` owns the node's error list (in memory, for the session); model inventories belong to the engines; manifests on disk are authored elsewhere. +- **Storage**: in-memory, plus the PATH records above. Manifests read from the per-user data dir's `engines/*.json` (`%LocalAppData%\Nvidia Corporation\Personal AI Router` on Windows, `~/.config/Nvidia Corporation/Personal AI Router` on Linux, `~/Library/Application Support/Nvidia Corporation/Personal AI Router` on macOS) plus bundled `manifests/*.json`. No database. +- **PATH record lifecycle**: written before the PATH change, so a crash mid-update is retryable. An engine uninstall that the user answered deletes the record, including one holding only the installed flag; an unasked one keeps a record that still holds entries and clears its installed flag. The application uninstaller (`--remove-user-path`, run only when the uninstaller deletes the data directory — `purge` on Debian) releases the entries but **keeps** each record's note that PAIR installed the engine, so a reinstall re-adopts it — an engine whose CLI lives outside PAIR's install directory has no other evidence distinguishing it from one the user installed. A record that exists but cannot be parsed is a failure for uninstall, not an absent claim; treating the two alike reported the entries released while stranding them. ## 10. Design Constraints - **Performance**: control plane, not inference; sub-second RPCs except install (network-bound) and start (bounded by the readiness timeout). - **Scalability**: a handful of engines per node; one managed instance per engine in v1. -- **Reliability**: best-effort; readiness + health probes; automatic restart on crash is planned but **not yet implemented** (see §4); install is one-shot and idempotent (detect short-circuits). +- **Reliability**: best-effort; readiness + health probes; automatic restart on crash is planned but **not yet implemented** (see §4); install is one-shot and idempotent — detect short-circuits the download, and a consented PATH step re-runs but changes nothing it already owns. - **Security**: **user mode only — no admin/sudo at runtime** (escalation reserved for product install time); both optional LAN listeners terminate pin-based mTLS and reject unpinned peers — the read-only model-list listener (`em`) because a node's model inventory is cluster data, and the `ec` control listener because its routes are privileged; `em` additionally serves plaintext on loopback only, for this node's own scanner; engines bind loopback by default, but a manifest's `runtime.bind` may open an inference engine to the LAN (Ollama defaults to `0.0.0.0`, overridable per-call); downloads are HTTPS-only (plain HTTP only from loopback) and checksum-verified before execution when the manifest pins a `sha256` (an unpinned fetch is HTTPS-only with a loud warning, like a `script` install). - **Compliance**: no PII; payloads carry engine/model identifiers and error messages only. @@ -177,6 +183,9 @@ The `engine:remote-*` methods are the client half: engine-manager resolves the t ## 12. Failure Modes and Mitigations - **Download / checksum failure on install**: engine stays NotInstalled. → Fail fast before `run`; `engine:install-progress` error + `errors:report` (`engine-manager:install-failed:`); never execute an unverified payload. +- **PATH publication fails on install**: the engine is installed and usable, but not by name. → Never fails the install, so the caller's start step still runs; `errors:report` (`engine-manager:path-failed:`) as a dismissible **warning** naming the manual alternative, cleared by a later install or uninstall that succeeds. Reported text folds the home directory to `~`, because a remote install streams it to the initiator and `nvpair-errors` push-syncs it to peers. +- **PATH cleanup fails on uninstall**: the executable is gone but the entry remains. → The receipt is kept and `errors:report` (`engine-manager:uninstall-failed:`) offers `retry`; a repeat `engine:uninstall` on an already-removed engine runs cleanup alone. Uninstalling the application drains every remaining receipt through `--remove-user-path`, since they live in the directory that uninstall deletes and the PATH entries do not. +- **Corrupt or lost PATH ownership record**: neither install nor uninstall can wedge on it. → An unparseable receipt is logged. Install treats it as no claim and writes a fresh one over it; uninstall treats it as a retryable failure, since reporting the entries released would strand them. A managed install with no receipt reacquires PATH rather than reporting a silent success. - **Engine fails its readiness probe**: start never reaches Running. → Bounded ready timeout → back to Stopped; structured error; no half-started state. - **Engine process crashes**: engine unavailable. → A watcher flips state, emits `engine:state-changed` + `errors:report` (`engine-manager:exited:`); the parent's supervisor reports if the *manager itself* dies. (Automatic restart is planned — see §4 — not yet implemented.) - **`nvpair-errors` unavailable**: failures absent from the node's error list. → The Broker no-ops the forward; local applog + ring buffers still hold everything. diff --git a/services/nvpair-engine-manager/status.go b/services/nvpair-engine-manager/status.go index 75419cb7..ea83109d 100644 --- a/services/nvpair-engine-manager/status.go +++ b/services/nvpair-engine-manager/status.go @@ -122,6 +122,7 @@ func (e *Executor) Errors() []serviceError { } func (e *Executor) snapshot(engine string, st *engineState) EngineStatus { + pathManaged := e.pathManaged(engine) st.mu.Lock() defer st.mu.Unlock() return EngineStatus{ @@ -131,6 +132,7 @@ func (e *Executor) snapshot(engine string, st *engineState) EngineStatus { Running: st.running, Healthy: st.healthy, Port: st.port, + PathManaged: pathManaged, } } diff --git a/services/nvpair-engine-manager/syncdir_unix.go b/services/nvpair-engine-manager/syncdir_unix.go new file mode 100644 index 00000000..f745320e --- /dev/null +++ b/services/nvpair-engine-manager/syncdir_unix.go @@ -0,0 +1,41 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +//go:build !windows + +package main + +import ( + "errors" + "log/slog" + "os" + "syscall" +) + +// syncDir flushes a directory entry so a rename into it survives a crash. +// os.Rename is atomic with respect to readers, but nothing guarantees the new +// entry has reached stable storage; ext4's auto_da_alloc and APFS hide that, +// XFS, btrfs, and networked home directories do not. +// +// A filesystem that rejects the flush outright is not a failure to report. The +// rename has already committed by the time this runs, so the file on disk is +// correct; turning EINVAL into an error would surface a PATH warning on install +// or a cleanup error on uninstall for a write that succeeded. CIFS and 9p do +// exactly that — network home directories and WSL interop paths, which is the +// case the flush above is aimed at. +func syncDir(dir string) error { + f, err := os.Open(dir) + if err != nil { + return err + } + syncErr := f.Sync() + if errors.Is(syncErr, syscall.EINVAL) || errors.Is(syncErr, syscall.ENOTSUP) { + slog.Debug("filesystem does not support flushing a directory", "dir", dir, "err", syncErr) + syncErr = nil + } + return errors.Join(syncErr, f.Close()) +} + +// replaceFile renames tmp over path. POSIX rename replaces a file other +// processes have open, so there is nothing to wait out. +func replaceFile(tmp, path string) error { return os.Rename(tmp, path) } diff --git a/services/nvpair-engine-manager/syncdir_windows.go b/services/nvpair-engine-manager/syncdir_windows.go new file mode 100644 index 00000000..f2145c87 --- /dev/null +++ b/services/nvpair-engine-manager/syncdir_windows.go @@ -0,0 +1,40 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +package main + +import ( + "errors" + "os" + "time" + + "golang.org/x/sys/windows" +) + +// syncDir is a no-op on Windows. There is no directory handle to flush: +// FlushFileBuffers rejects one, and NTFS journals the rename's metadata itself. +func syncDir(string) error { return nil } + +const ( + replaceAttempts = 20 + replaceBackoff = 25 * time.Millisecond +) + +// replaceFile renames tmp over path. Go opens files without FILE_SHARE_DELETE, +// so while any process is reading the target — a status snapshot reads PATH +// ownership records without a lock — MoveFileEx fails with a sharing violation. +// Those reads are short, so the rename waits them out instead of failing. +func replaceFile(tmp, path string) error { + var err error + for attempt := 0; attempt < replaceAttempts; attempt++ { + if err = os.Rename(tmp, path); err == nil || !transientReplaceError(err) { + return err + } + time.Sleep(replaceBackoff) + } + return err +} + +func transientReplaceError(err error) bool { + return errors.Is(err, windows.ERROR_SHARING_VIOLATION) || errors.Is(err, windows.ERROR_ACCESS_DENIED) +} diff --git a/services/nvpair-engine-manager/syncdir_windows_test.go b/services/nvpair-engine-manager/syncdir_windows_test.go new file mode 100644 index 00000000..f3352d45 --- /dev/null +++ b/services/nvpair-engine-manager/syncdir_windows_test.go @@ -0,0 +1,46 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +package main + +import ( + "os" + "path/filepath" + "testing" + "time" +) + +// A status snapshot reads an ownership record without the PATH lock. A write +// that lands during that read must wait it out, not fail. +func TestReplaceFileWaitsOutAnOpenReader(t *testing.T) { + dir := t.TempDir() + target := filepath.Join(dir, "record.json") + tmp := filepath.Join(dir, "record.json.tmp") + if err := os.WriteFile(target, []byte("old"), 0o600); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(tmp, []byte("new"), 0o600); err != nil { + t.Fatal(err) + } + reader, err := os.Open(target) + if err != nil { + t.Fatal(err) + } + released := make(chan struct{}) + go func() { + time.Sleep(4 * replaceBackoff) + _ = reader.Close() + close(released) + }() + if err := replaceFile(tmp, target); err != nil { + t.Fatalf("replace while a reader held the file: %v", err) + } + <-released + data, err := os.ReadFile(target) + if err != nil { + t.Fatal(err) + } + if string(data) != "new" { + t.Fatalf("target = %q, want the replacement", data) + } +} diff --git a/services/nvpair-engine-manager/testdata/fakeengine/main.go b/services/nvpair-engine-manager/testdata/fakeengine/main.go index 9f12ba82..af128e21 100644 --- a/services/nvpair-engine-manager/testdata/fakeengine/main.go +++ b/services/nvpair-engine-manager/testdata/fakeengine/main.go @@ -19,6 +19,7 @@ import ( "net" "net/http" "os" + "path/filepath" "strconv" "strings" "sync" @@ -108,9 +109,29 @@ func main() { os.Exit(1) } return - case "touch": // write a marker file so a test can assert the command ran - if len(os.Args) > 2 { - _ = os.WriteFile(os.Args[2], []byte("ok"), 0o644) + case "remove": // delete every named file, standing in for an uninstaller + for _, path := range os.Args[2:] { + if err := os.Remove(path); err != nil && !os.IsNotExist(err) { + log.Fatal(err) + } + } + return + case "write-env": // record an environment variable's value, to prove the + // installer subprocess received a manifest-declared override + if len(os.Args) < 4 { + fmt.Fprintln(os.Stderr, "write-env: need ") + os.Exit(2) + } + if err := os.WriteFile(os.Args[3], []byte(os.Getenv(os.Args[2])), 0o644); err != nil { + log.Fatal(err) + } + return + case "touch": // write marker files so a test can assert the command ran + for _, path := range os.Args[2:] { + if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { + log.Fatal(err) + } + _ = os.WriteFile(path, []byte("ok"), 0o644) } return case "echo": // print args to stdout so a cmd-action can capture output diff --git a/services/nvpair-engine-manager/userpath.go b/services/nvpair-engine-manager/userpath.go new file mode 100644 index 00000000..0f01a0c7 --- /dev/null +++ b/services/nvpair-engine-manager/userpath.go @@ -0,0 +1,58 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +package main + +import ( + "fmt" + "os" + "path/filepath" + "strings" + "sync" +) + +// The OS-neutral entry point for user PATH changes, and the contract each +// platform implements. +// +// Two functions, both build-tagged, one pair per platform: +// +// persistUserPath(dir, receipt, save) error — publish dir on the user's PATH +// removeUserPath(receipt) error — withdraw what the receipt claims +// +// Windows implements them in userpath_windows.go against HKCU\Environment\Path; +// everything else in userpath_unix.go against the login shell's profiles. +// Both are always called with the user PATH lock held (see lockUserPath), which +// covers the whole read-modify-write, not just the write. +// +// Ownership rules the two share: +// +// - Record before writing, so a crash or a failed write is retryable. +// - Never claim an entry PAIR did not put there. Uninstall deletes exactly +// what the receipt names, so a wrong claim deletes a user's own entry. +// +// Where they legitimately differ: Windows records only when it actually +// appends, because an entry already present is indistinguishable from the +// user's own. Unix records even when the profile already holds a byte-identical +// block, because PAIR's block is self-identifying — it is demonstrably the +// author, and re-adopting is what lets a reinstall clean up after a lost +// receipt. +// +// A third platform implements the two functions and nothing else. The shell +// editing in userpath_shell.go and the Windows entry parsing in +// userpath_windows_entries.go are deliberately untagged so their rules stay +// testable everywhere, not because either is portable. + +// Both engines can finish installing together. Serialize user PATH updates +// across engines so one read/modify/write cannot discard the other's entry. +// This covers one process only; lockUserPath extends it across the several +// engine-managers a machine can run at once. +var userPathMu sync.Mutex + +// addToUserPath validates the directory before changing the current user's PATH. +// The caller holds the user PATH lock through both ownership and environment writes. +func addToUserPath(dir string, receipt *pathReceipt, save func() error) error { + if !filepath.IsAbs(dir) || strings.ContainsAny(dir, "\r\n\x00"+string(os.PathListSeparator)) { + return fmt.Errorf("invalid command-line directory %q", dir) + } + return persistUserPath(dir, receipt, save) +} diff --git a/services/nvpair-engine-manager/userpath_shell.go b/services/nvpair-engine-manager/userpath_shell.go new file mode 100644 index 00000000..f75afcc3 --- /dev/null +++ b/services/nvpair-engine-manager/userpath_shell.go @@ -0,0 +1,231 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +package main + +import ( + "errors" + "fmt" + "os" + "path/filepath" + "strings" + "time" +) + +// Keep profile editing independent of the host OS so tests can use temporary +// homes on every platform, without touching the developer's shell configuration. +func addToShellPath(home, shell, zdotdir, configHome, dir string, receipt *pathReceipt, save func() error) error { + quote := "'" + strings.ReplaceAll(dir, "'", "'\"'\"'") + "'" + block := "\n# PAIR engine command-line tools\ncase \":$PATH:\" in\n *:" + quote + ":*) ;;\n *) export PATH=\"${PATH:+$PATH:}\"" + quote + " ;;\nesac\n" + var profiles []string + switch filepath.Base(shell) { + case "zsh": + if zdotdir == "" { + zdotdir = home + } + profiles = []string{filepath.Join(zdotdir, ".zprofile"), filepath.Join(zdotdir, ".zshrc")} + case "bash": + login := filepath.Join(home, ".profile") + for _, name := range []string{".bash_profile", ".bash_login"} { + candidate := filepath.Join(home, name) + if _, err := os.Stat(candidate); err == nil { + login = candidate + break + } else if !errors.Is(err, os.ErrNotExist) { + return err + } + } + profiles = []string{login, filepath.Join(home, ".bashrc")} + case "fish": + if configHome == "" { + configHome = filepath.Join(home, ".config") + } + profiles = []string{filepath.Join(configHome, "fish", "conf.d", "nvpair-path.fish")} + quote = "'" + strings.NewReplacer(`\`, `\\`, "'", `\'`).Replace(dir) + "'" + block = "\n# PAIR engine command-line tools\nif not contains -- " + quote + " $PATH\n set -gx PATH $PATH " + quote + "\nend\n" + case "sh", "dash", "ksh", "ash", "busybox": + profiles = []string{filepath.Join(home, ".profile")} + default: + // tcsh, nushell, elvish, xonsh and friends each need their own syntax. + // The caller reports this as a warning with the directory to add, so an + // unrecognized shell costs the user one manual step, not the install. + return fmt.Errorf("automatic PATH setup is not supported for the %s shell", filepath.Base(shell)) + } + for _, profile := range profiles { + profile, err := filepath.Abs(profile) + if err != nil { + return err + } + if err := appendPathBlock(profile, block, receipt, save); err != nil { + return fmt.Errorf("update %s: %w", filepath.Base(profile), err) + } + } + return nil +} + +// appendPathBlock records the snippet as ours before writing it. +// +// Ownership is recorded even when the profile already carries a byte-identical +// block, because PAIR is demonstrably the author: a reinstall after the receipt +// was lost — wiping the data directory deletes it — must be able to re-adopt +// the block, or the next uninstall reports success while leaving it behind. +func appendPathBlock(profile, block string, receipt *pathReceipt, save func() error) error { + // Recorded before the profile is read rather than between reading it and + // rewriting it. save() is two fsyncs, and every one of them sat inside the + // window in which a concurrent editor's save to the same profile would be + // discarded by the rewrite below. + owned := pathBlock{Profile: profile, Text: block} + recorded := false + for _, previous := range receipt.ShellBlocks { + recorded = recorded || previous == owned + } + if !recorded { + receipt.ShellBlocks = append(receipt.ShellBlocks, owned) + if err := save(); err != nil { + return err + } + } + contents, before, err := readProfile(profile) + if err != nil { + return err + } + if strings.Contains(string(contents), block) { + return nil + } + return writeShellProfile(profile, append(contents, block...), before) +} + +// removeShellPath removes exact recorded snippets and preserves other content. +func removeShellPath(receipt *pathReceipt) error { + for _, block := range receipt.ShellBlocks { + contents, before, err := readProfile(block.Profile) + if err != nil { + // Named by basename: this message reaches cluster peers. + return fmt.Errorf("read %s: %w", filepath.Base(block.Profile), err) + } + // A user-edited block is no longer ours to delete. + if block.Text == "" || !strings.Contains(string(contents), block.Text) { + continue + } + next := strings.Replace(string(contents), block.Text, "", 1) + if err := writeShellProfile(block.Profile, []byte(next), before); err != nil { + return fmt.Errorf("update %s: %w", filepath.Base(block.Profile), err) + } + } + return nil +} + +// profileSnapshot fingerprints a profile so a rewrite can tell whether anything +// else changed it since it was read. +type profileSnapshot struct { + size int64 + modTime time.Time + absent bool +} + +func statProfile(profile string) (profileSnapshot, error) { + info, err := os.Stat(profile) + if errors.Is(err, os.ErrNotExist) { + return profileSnapshot{absent: true}, nil + } + if err != nil { + return profileSnapshot{}, err + } + return profileSnapshot{size: info.Size(), modTime: info.ModTime()}, nil +} + +// errProfileChanged reports that someone else wrote to the profile mid-update. +// Both callers surface it as a retryable failure, which is the right answer: +// the next attempt reads the user's new contents and appends to those. +var errProfileChanged = errors.New("the file changed while PAIR was updating it") + +// readProfile fingerprints a profile before reading it. Taking the stat first +// means a write that races the read fails the later comparison instead of +// passing it — the worst case is a spurious retry, never a silent overwrite. +func readProfile(profile string) ([]byte, profileSnapshot, error) { + before, err := statProfile(profile) + if err != nil { + return nil, profileSnapshot{}, err + } + contents, err := os.ReadFile(profile) + if err != nil && !errors.Is(err, os.ErrNotExist) { + return nil, profileSnapshot{}, err + } + return contents, before, nil +} + +// writeShellProfile replaces a profile in one step: write a sibling temporary +// file, flush it, then rename it over the original. Both directions go through +// here. An append cannot use O_APPEND, because a short write — ENOSPC, a quota — +// would leave the user a profile truncated mid-statement that every new shell +// then fails to parse, and that PAIR could neither match nor repair afterwards. +// The flush matters for the same reason: rename swaps the directory entry +// atomically but says nothing about the data reaching stable storage, and PAIR +// did not create these files so it has no copy to restore. +// +// Symlinks are resolved so a dotfile linked into a dotfiles repository is +// updated in place rather than replaced by a regular file. +// +// The cost is inode identity: the replacement carries the permission bits over +// but not ACLs, extended attributes, or additional hard links. That is accepted +// deliberately. Preserving them means writing through the original inode, which +// is exactly the short-write hazard above, and the alternative — copying each +// class of metadata — is platform-specific, incomplete, and guards a dotfile +// arrangement far rarer than a full disk. Do not turn this back into an +// in-place write. +// +// since is the fingerprint readProfile took, re-checked immediately before the +// rename. Replacing the whole file from a copy read earlier would discard an +// edit that landed in between, and the PAIR lock cannot prevent that one — the +// competing writer is the user's editor, not another engine-manager. +func writeShellProfile(profile string, contents []byte, since profileSnapshot) error { + target := profile + perm := os.FileMode(0o644) + switch resolved, err := filepath.EvalSymlinks(profile); { + case err == nil: + info, statErr := os.Stat(resolved) + if statErr != nil { + return statErr + } + target, perm = resolved, info.Mode().Perm() + case !errors.Is(err, os.ErrNotExist): + return err + } + dir := filepath.Dir(target) + if err := os.MkdirAll(dir, 0o755); err != nil { + return err + } + f, err := os.CreateTemp(dir, ".pair-path-*") + if err != nil { + return err + } + defer os.Remove(f.Name()) + if err := writeAndSync(f, contents, perm); err != nil { + return err + } + switch now, err := statProfile(profile); { + case err != nil: + return err + case now != since: + return fmt.Errorf("%w: %s", errProfileChanged, filepath.Base(profile)) + } + if err := os.Rename(f.Name(), target); err != nil { + return err + } + return syncDir(dir) +} + +// writeAndSync fills an open file and closes it durably, so the caller's rename +// publishes the bytes rather than an empty file. +func writeAndSync(f *os.File, contents []byte, perm os.FileMode) error { + if err := f.Chmod(perm); err != nil { + return errors.Join(err, f.Close()) + } + if _, err := f.Write(contents); err != nil { + return errors.Join(err, f.Close()) + } + if err := f.Sync(); err != nil { + return errors.Join(err, f.Close()) + } + return f.Close() +} diff --git a/services/nvpair-engine-manager/userpath_test.go b/services/nvpair-engine-manager/userpath_test.go new file mode 100644 index 00000000..e4db1b27 --- /dev/null +++ b/services/nvpair-engine-manager/userpath_test.go @@ -0,0 +1,238 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +package main + +import ( + "errors" + "os" + "os/exec" + "path/filepath" + "strings" + "testing" +) + +func TestAppendWindowsPath(t *testing.T) { + expand := func(s string) string { return strings.ReplaceAll(s, "%USERPROFILE%", `C:\Users\Test`) } + test := func(name, current, dir, want string) { + t.Run(name, func(t *testing.T) { + if got := appendWindowsPath(current, dir, expand); got != want { + t.Fatalf("PATH = %q, want %q", got, want) + } + }) + } + test("first entry", "", `C:\Engine Tools`, `C:\Engine Tools`) + test("preserves order and references", `%USERPROFILE%\bin;C:\Tools`, `C:\Engine`, `%USERPROFILE%\bin;C:\Tools;C:\Engine`) + test("case insensitive existing directory", `C:\TOOLS\;D:\Other`, `c:\tools`, `C:\TOOLS\;D:\Other`) + test("existing expanded directory", `%USERPROFILE%\bin`, `C:\Users\Test\bin`, `%USERPROFILE%\bin`) + test("existing quoted directory", `"C:\Engine Tools"`, `C:\Engine Tools`, `"C:\Engine Tools"`) + // Windows ignores a trailing empty element but searches an interior one, and + // has historically resolved it against the current directory. + test("trailing separator", `C:\Tools;`, `C:\Engine`, `C:\Tools;C:\Engine`) + test("only separators", `;;`, `C:\Engine`, `C:\Engine`) + test("different directory with same prefix", `C:\Engine-old`, `C:\Engine`, `C:\Engine-old;C:\Engine`) +} + +func TestShellPathNamesAnUnsupportedShell(t *testing.T) { + err := testAddToShellPath(t.TempDir(), "/usr/local/bin/nu", "", "", "/opt/bin") + if err == nil || !strings.Contains(err.Error(), "nu") { + t.Fatalf("error = %v, want the shell named", err) + } +} + +// Every shell PAIR claims to support has to actually get a profile written. +func TestShellPathSupportsPosixShellFamilies(t *testing.T) { + for _, shell := range []string{"sh", "dash", "ksh", "ash", "busybox"} { + t.Run(shell, func(t *testing.T) { + home := t.TempDir() + if err := testAddToShellPath(home, "/bin/"+shell, "", "", "/opt/bin"); err != nil { + t.Fatal(err) + } + if _, err := os.Stat(filepath.Join(home, ".profile")); err != nil { + t.Fatal(err) + } + }) + } +} + +func TestShellPathProfilesPreserveContentAndAreIdempotent(t *testing.T) { + test := func(name, shell string, profiles []string) { + t.Run(name, func(t *testing.T) { + home := t.TempDir() + dir := "/opt/Engine Tools/bin" + const original = "# existing user configuration\n" + for _, profile := range profiles { + filename := filepath.Join(home, profile) + if err := os.MkdirAll(filepath.Dir(filename), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filename, []byte(original), 0o644); err != nil { + t.Fatal(err) + } + } + if err := testAddToShellPath(home, shell, "", "", dir); err != nil { + t.Fatal(err) + } + first := make(map[string]string) + for _, profile := range profiles { + data, err := os.ReadFile(filepath.Join(home, profile)) + if err != nil { + t.Fatal(err) + } + first[profile] = string(data) + if !strings.HasPrefix(string(data), original) || !strings.Contains(string(data), dir) { + t.Fatalf("profile content = %q", data) + } + } + if err := testAddToShellPath(home, shell, "", "", dir); err != nil { + t.Fatal(err) + } + for _, profile := range profiles { + data, err := os.ReadFile(filepath.Join(home, profile)) + if err != nil { + t.Fatal(err) + } + if string(data) != first[profile] { + t.Fatalf("repeat changed %s", profile) + } + } + }) + } + test("bash login and interactive terminals", "/bin/bash", []string{".profile", ".bashrc"}) + test("bash honors existing login profile", "/bin/bash", []string{".bash_profile", ".bashrc"}) + test("zsh login and interactive terminals", "/bin/zsh", []string{".zprofile", ".zshrc"}) + test("fish configuration", "/usr/bin/fish", []string{filepath.Join(".config", "fish", "conf.d", "nvpair-path.fish")}) +} + +func TestShellPathUsesConfiguredProfileDirectories(t *testing.T) { + home, zdir, config := t.TempDir(), t.TempDir(), t.TempDir() + if err := testAddToShellPath(home, "zsh", zdir, "", "/opt/ollama/bin"); err != nil { + t.Fatal(err) + } + if _, err := os.Stat(filepath.Join(zdir, ".zshrc")); err != nil { + t.Fatal(err) + } + if err := testAddToShellPath(home, "fish", "", config, "/opt/lms/bin"); err != nil { + t.Fatal(err) + } + if _, err := os.Stat(filepath.Join(config, "fish", "conf.d", "nvpair-path.fish")); err != nil { + t.Fatal(err) + } + entries, err := os.ReadDir(home) + if err != nil { + t.Fatal(err) + } + if len(entries) != 0 { + t.Fatalf("wrote to home instead of configured directories: %v", entries) + } +} + +func TestShellPathReportsUnwritableProfile(t *testing.T) { + home := t.TempDir() + if err := os.Mkdir(filepath.Join(home, ".profile"), 0o755); err != nil { + t.Fatal(err) + } + if err := testAddToShellPath(home, "bash", "", "", "/opt/bin"); err == nil { + t.Fatal("expected a profile write failure") + } +} + +func TestShellPathPreservesLiteralDirectoriesAtRuntime(t *testing.T) { + shell, err := exec.LookPath("sh") + if err != nil { + t.Skip("POSIX shell unavailable") + } + test := func(name, dir string) { + t.Run(name, func(t *testing.T) { + home := t.TempDir() + if err := testAddToShellPath(home, "sh", "", "", dir); err != nil { + t.Fatal(err) + } + profile, err := os.ReadFile(filepath.Join(home, ".profile")) + if err != nil { + t.Fatal(err) + } + // Source twice to check runtime deduplication, not just file deduplication. + command := "PATH=/existing\n" + string(profile) + string(profile) + "printf '%s' \"$PATH\"" + out, err := exec.Command(shell, "-c", command).CombinedOutput() + if err != nil { + t.Fatalf("source profile: %v: %s", err, out) + } + if string(out) != "/existing:"+dir { + t.Fatalf("PATH = %q, want literal directory %q appended once", out, dir) + } + }) + } + test("spaces", "/opt/Engine Tools/bin") + test("quotes and shell metacharacters", "/opt/it's $(printf injected) [engine]/bin") +} + +// Profile-only tests use temporary homes and do not need a persistent receipt. +func testAddToShellPath(home, shell, zdotdir, configHome, dir string) error { + return addToShellPath(home, shell, zdotdir, configHome, dir, &pathReceipt{}, func() error { return nil }) +} + +// A dotfile is often a symlink into a dotfiles repository. Replacing the link +// with a regular file would detach the user's profile from the repository that +// manages it, so the write has to go through to the target. +func TestShellProfileWriteFollowsASymlink(t *testing.T) { + home := t.TempDir() + store := t.TempDir() + target := filepath.Join(store, "profile") + if err := os.WriteFile(target, []byte("# managed elsewhere\n"), 0o644); err != nil { + t.Fatal(err) + } + link := filepath.Join(home, ".profile") + if err := os.Symlink(target, link); err != nil { + t.Skipf("symlinks unavailable: %v", err) + } + if err := testAddToShellPath(home, "sh", "", "", "/opt/engine/bin"); err != nil { + t.Fatal(err) + } + info, err := os.Lstat(link) + if err != nil { + t.Fatal(err) + } + if info.Mode()&os.ModeSymlink == 0 { + t.Error("the profile symlink was replaced by a regular file") + } + contents, err := os.ReadFile(target) + if err != nil { + t.Fatal(err) + } + if !strings.Contains(string(contents), "/opt/engine/bin") { + t.Errorf("the symlink target was not updated: %q", contents) + } + if !strings.Contains(string(contents), "# managed elsewhere") { + t.Errorf("the symlink target lost its original contents: %q", contents) + } +} + +// The rewrite replaces the whole file from a copy read earlier, and the PAIR +// lock cannot cover the competing writer here — it is the user's editor. An +// edit that lands in the gap has to be noticed rather than discarded. +func TestShellProfileWriteRefusesToDiscardAConcurrentEdit(t *testing.T) { + profile := filepath.Join(t.TempDir(), ".profile") + if err := os.WriteFile(profile, []byte("original\n"), 0o644); err != nil { + t.Fatal(err) + } + contents, before, err := readProfile(profile) + if err != nil { + t.Fatal(err) + } + edited := "original\nadded by the user's editor\n" + if err := os.WriteFile(profile, []byte(edited), 0o644); err != nil { + t.Fatal(err) + } + err = writeShellProfile(profile, append(contents, "# PAIR\n"...), before) + if !errors.Is(err, errProfileChanged) { + t.Fatalf("write returned %v, want the concurrent edit to be refused", err) + } + after, err := os.ReadFile(profile) + if err != nil { + t.Fatal(err) + } + if string(after) != edited { + t.Errorf("the user's edit was overwritten: %q", after) + } +} diff --git a/services/nvpair-engine-manager/userpath_unix.go b/services/nvpair-engine-manager/userpath_unix.go new file mode 100644 index 00000000..96b092ec --- /dev/null +++ b/services/nvpair-engine-manager/userpath_unix.go @@ -0,0 +1,222 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +//go:build !windows + +package main + +import ( + "bufio" + "context" + "errors" + "log/slog" + "os" + "os/exec" + "os/user" + "path/filepath" + "runtime" + "strings" + "time" +) + +// persistUserPath selects the installing user's shell configuration. +func persistUserPath(dir string, receipt *pathReceipt, save func() error) error { + home, err := os.UserHomeDir() + if err != nil { + return err + } + shell := loginShell() + // Resolved only for zsh. An unreadable .zshenv has to fail rather than fall + // back, and a bash user's install must not be the thing it fails. + zdotdir := "" + if filepath.Base(shell) == "zsh" { + if zdotdir, err = zshDotDir(home); err != nil { + return err + } + } + return addToShellPath(home, shell, zdotdir, os.Getenv("XDG_CONFIG_HOME"), dir, receipt, save) +} + +// removeUserPath uses recorded profiles, even if the user has changed shells. +func removeUserPath(receipt *pathReceipt) error { + return removeShellPath(receipt) +} + +// loginShell reports the shell the user actually logs in with, from the account +// database rather than $SHELL. +// +// Engine-manager is spawned by the broker, which the desktop application +// spawns, so its environment is the GUI session's. $SHELL is usually right +// there but is not guaranteed to be set at all, and writing to the wrong +// shell's profiles succeeds silently while leaving nothing on PATH. +func loginShell() string { + u, err := user.Current() + if err != nil { + slog.Debug("no account record for the current user", "err", err) + return environmentShell() + } + if shell := accountShell(u); shell != "" { + return shell + } + return environmentShell() +} + +// accountShell reads the login shell out of the passwd database. On Linux the +// file covers ordinary users; on macOS it holds only system accounts, so a real +// account has to come from Directory Services. +func accountShell(u *user.User) string { + if runtime.GOOS == "darwin" { + return directoryServiceShell(u.Username) + } + if shell := passwdFileShell("/etc/passwd", u.Username); shell != "" { + return shell + } + // NSS-backed accounts (LDAP, SSSD) are absent from the file. + getent, err := exec.LookPath("getent") + if err != nil { + return "" + } + return passwdEntryShell(probeAccountDatabase(getent, "passwd", u.Username), u.Username) +} + +func directoryServiceShell(username string) string { + out := probeAccountDatabase("/usr/bin/dscl", ".", "-read", "/Users/"+username, "UserShell") + _, value, ok := strings.Cut(out, ":") + if !ok { + return "" + } + return strings.TrimSpace(value) +} + +// accountProbeTimeout bounds a single account-database lookup. +// +// getent blocks on an unreachable LDAP or SSSD backend and dscl on a directory +// server that is not answering, both of which are ordinary on a laptop off the +// corporate network. This runs inside an install whose PATH step is supposed to +// degrade to a warning, so a hung probe has to become a miss rather than an +// install that never returns. +const accountProbeTimeout = 3 * time.Second + +// probeAccountDatabase returns the command's trimmed output, or "" if it fails +// or outlasts its budget. The caller falls back to the environment. +func probeAccountDatabase(name string, args ...string) string { + ctx, cancel := context.WithTimeout(context.Background(), accountProbeTimeout) + defer cancel() + cmd := exec.CommandContext(ctx, name, args...) + // Killing the probe closes the pipe Output is reading only if nothing it + // spawned still holds the other end. WaitDelay caps that too, so the + // deadline is on returning rather than on the process alone. + cmd.WaitDelay = accountProbeTimeout + out, err := cmd.Output() + if err != nil { + slog.Debug("account database lookup failed", "command", name, "err", err) + return "" + } + return strings.TrimSpace(string(out)) +} + +func passwdFileShell(file, username string) string { + f, err := os.Open(file) + if err != nil { + slog.Debug("could not read the passwd database", "file", file, "err", err) + return "" + } + defer f.Close() + scanner := bufio.NewScanner(f) + for scanner.Scan() { + if shell := passwdEntryShell(scanner.Text(), username); shell != "" { + return shell + } + } + // "Not listed" and "stopped looking" are different facts. The caller falls + // back either way, but only one of them explains a profile written to the + // wrong shell, so it has to reach the log. + if err := scanner.Err(); err != nil { + slog.Debug("stopped reading the passwd database early", "file", file, "err", err) + } + return "" +} + +// passwdEntryShell returns field 7 of a passwd line belonging to username. +func passwdEntryShell(line, username string) string { + fields := strings.Split(line, ":") + if len(fields) < 7 || fields[0] != username { + return "" + } + return strings.TrimSpace(fields[6]) +} + +// environmentShell is the last resort when the account database is unreadable. +func environmentShell() string { + if shell := os.Getenv("SHELL"); shell != "" { + return shell + } + if runtime.GOOS == "darwin" { + return "zsh" + } + return "bash" +} + +// zshDotDir resolves where zsh looks for .zprofile and .zshrc. +// +// ZDOTDIR is conventionally assigned in ~/.zshenv, which zsh reads before +// anything else, so it is essentially never visible in this process's inherited +// environment. Missing it writes two stray dotfiles into $HOME that the user's +// zsh never reads, and reports success. +func zshDotDir(home string) (string, error) { + if dir := os.Getenv("ZDOTDIR"); dir != "" { + return dir, nil + } + return zshenvDotDir(home) +} + +// zshenvDotDir scans ~/.zshenv for a plain ZDOTDIR assignment. Only a literal +// value is honoured: anything built from a command substitution or a conditional +// cannot be evaluated here, and guessing at it would be worse than falling back +// to $HOME. +func zshenvDotDir(home string) (string, error) { + data, err := os.ReadFile(filepath.Join(home, ".zshenv")) + if errors.Is(err, os.ErrNotExist) { + return "", nil + } + if err != nil { + // Falling back to $HOME on a permission or I/O error would write two + // dotfiles the user's zsh never reads and report success — the exact + // outcome this function exists to prevent, on evidence it never got. + return "", err + } + found := "" + for _, line := range strings.Split(string(data), "\n") { + statement := strings.TrimSpace(strings.TrimPrefix(strings.TrimSpace(line), "export ")) + value, ok := strings.CutPrefix(statement, "ZDOTDIR=") + if !ok { + continue + } + value = strings.TrimSpace(value) + if len(value) >= 2 && (value[0] == '"' || value[0] == '\'') && value[len(value)-1] == value[0] { + value = value[1 : len(value)-1] + } else if cut := strings.IndexAny(value, " \t"); cut >= 0 { + // Unquoted, so zsh ends the word at the first space and the rest is + // a trailing comment. Without this, `ZDOTDIR=$HOME/.config/zsh # tidy` + // cleared every guard below and PAIR created a directory named + // `zsh # tidy`. + value = value[:cut] + } + value = expandHomePrefix(home, value) + if !filepath.IsAbs(value) || strings.ContainsAny(value, "$`(){}") { + continue + } + found = value // A later assignment wins, as it would in zsh. + } + return found, nil +} + +// expandHomePrefix resolves the only references a static read can resolve. +func expandHomePrefix(home, value string) string { + for _, prefix := range []string{"$HOME/", "${HOME}/", "~/"} { + if rest, ok := strings.CutPrefix(value, prefix); ok { + return filepath.Join(home, rest) + } + } + return value +} diff --git a/services/nvpair-engine-manager/userpath_unix_test.go b/services/nvpair-engine-manager/userpath_unix_test.go new file mode 100644 index 00000000..8d7531d8 --- /dev/null +++ b/services/nvpair-engine-manager/userpath_unix_test.go @@ -0,0 +1,79 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +//go:build !windows + +package main + +import ( + "os" + "path/filepath" + "testing" +) + +// Getting ZDOTDIR wrong is not a cosmetic error: PAIR writes .zprofile and +// .zshrc into the directory this returns, so a wrong answer leaves two stray +// dotfiles the user's zsh never reads and reports the PATH entry published. +func TestZshenvDotDir(t *testing.T) { + test := func(name, zshenv, want string) { + t.Run(name, func(t *testing.T) { + home := t.TempDir() + if err := os.WriteFile(filepath.Join(home, ".zshenv"), []byte(zshenv), 0o644); err != nil { + t.Fatal(err) + } + got, err := zshenvDotDir(home) + if err != nil { + t.Fatal(err) + } + if want == "$HOME/.config/zsh" { + want = filepath.Join(home, ".config", "zsh") + } + if got != want { + t.Errorf("ZDOTDIR = %q, want %q", got, want) + } + }) + } + test("absent assignment", "# nothing here\n", "") + test("plain assignment", "ZDOTDIR=$HOME/.config/zsh\n", "$HOME/.config/zsh") + test("exported assignment", "export ZDOTDIR=$HOME/.config/zsh\n", "$HOME/.config/zsh") + test("double quoted", "export ZDOTDIR=\"$HOME/.config/zsh\"\n", "$HOME/.config/zsh") + test("single quoted", "export ZDOTDIR='$HOME/.config/zsh'\n", "$HOME/.config/zsh") + // zsh ends an unquoted word at the first space, so the comment is not part + // of the directory. Treating it as one made PAIR create `zsh # keep tidy`. + test("trailing comment", "export ZDOTDIR=$HOME/.config/zsh # keep tidy\n", "$HOME/.config/zsh") + // A later assignment wins, as it would in zsh. + test("reassigned", "ZDOTDIR=$HOME/first\nZDOTDIR=$HOME/.config/zsh\n", "$HOME/.config/zsh") + // Anything PAIR cannot evaluate the way zsh would is declined rather than + // guessed at: the fallback to $HOME is the safe answer. + test("command substitution", "export ZDOTDIR=$(dirname /a/b)\n", "") + test("parameter expansion", "export ZDOTDIR=${XDG_CONFIG_HOME}/zsh\n", "") + test("relative path", "export ZDOTDIR=.config/zsh\n", "") +} + +// An unreadable .zshenv is not an absent one. Falling back to $HOME on a +// permission error produces exactly the stray-dotfile outcome above, while +// reporting success. +func TestZshenvDotDirDistinguishesUnreadableFromAbsent(t *testing.T) { + t.Run("absent", func(t *testing.T) { + got, err := zshenvDotDir(t.TempDir()) + if err != nil { + t.Fatalf("an absent .zshenv is not an error: %v", err) + } + if got != "" { + t.Errorf("ZDOTDIR = %q, want the caller to fall back to $HOME", got) + } + }) + t.Run("unreadable", func(t *testing.T) { + if os.Geteuid() == 0 { + t.Skip("root reads a 0o000 file regardless of its mode") + } + home := t.TempDir() + zshenv := filepath.Join(home, ".zshenv") + if err := os.WriteFile(zshenv, []byte("export ZDOTDIR=$HOME/.config/zsh\n"), 0o000); err != nil { + t.Fatal(err) + } + if _, err := zshenvDotDir(home); err == nil { + t.Error("an unreadable .zshenv was reported as absent") + } + }) +} diff --git a/services/nvpair-engine-manager/userpath_windows.go b/services/nvpair-engine-manager/userpath_windows.go new file mode 100644 index 00000000..ffd0ab13 --- /dev/null +++ b/services/nvpair-engine-manager/userpath_windows.go @@ -0,0 +1,103 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +package main + +import ( + "errors" + "log/slog" + "runtime" + "unsafe" + + "golang.org/x/sys/windows" + "golang.org/x/sys/windows/registry" +) + +// Resolved once. Building the lazy DLL per call re-resolves the module and +// leaks a reference for every PATH update. +var ( + user32 = windows.NewLazySystemDLL("user32.dll") + procSendMessageTimeoutW = user32.NewProc("SendMessageTimeoutW") +) + +// persistUserPath appends a user PATH entry only after recording its ownership. +func persistUserPath(dir string, receipt *pathReceipt, save func() error) error { + return updateWindowsPath(func(current string) (string, error) { + return addWindowsPath(current, dir, expandPath, receipt, save) + }) +} + +// removeUserPath removes only the registry entry recorded for this installation. +func removeUserPath(receipt *pathReceipt) error { + if receipt.WindowsEntry == "" { + return nil + } + return updateWindowsPath(func(current string) (string, error) { + return removeWindowsPath(current, receipt.WindowsEntry), nil + }) +} + +// updateWindowsPath preserves the registry value type and notifies new processes. +func updateWindowsPath(update func(string) (string, error)) error { + key, err := registry.OpenKey(registry.CURRENT_USER, `Environment`, registry.QUERY_VALUE|registry.SET_VALUE) + if err != nil { + return err + } + defer key.Close() + current, kind, err := key.GetStringValue("Path") + if errors.Is(err, registry.ErrNotExist) { + current, kind, err = "", registry.EXPAND_SZ, nil + } + if err != nil { + return err + } + next, err := update(current) + if err != nil { + return err + } + if next == current { + return nil + } + if kind == registry.EXPAND_SZ { + err = key.SetExpandStringValue("Path", next) + } else { + err = key.SetStringValue("Path", next) + } + if err != nil { + return err + } + broadcastEnvironmentChange() + return nil +} + +// broadcastEnvironmentChange tells Explorer that future processes should pick up +// the new user PATH. Failure is logged, never returned: SMTO_ABORTIFHUNG makes +// SendMessageTimeoutW return 0 whenever any top-level window is not pumping +// messages — routine on a busy desktop, and certain in a session with no window +// station — while the registry value it is announcing is already authoritative +// for every shell started from here on. +func broadcastEnvironmentChange() { + name, err := windows.UTF16PtrFromString("Environment") + if err != nil { + slog.Warn("skipped the environment change broadcast", "err", err) + return + } + const ( + hwndBroadcast = 0xffff + wmSettingChange = 0x001a + smtoAbortIfHung = 0x0002 + timeoutMs = 5000 + ) + // The final lpdwResult is optional and nothing here reads it, so pass NULL + // rather than a pointer into a variadic call that offers no lifetime guarantee. + delivered, _, callErr := procSendMessageTimeoutW.Call( + hwndBroadcast, wmSettingChange, 0, uintptr(unsafe.Pointer(name)), + smtoAbortIfHung, timeoutMs, 0, + ) + runtime.KeepAlive(name) + if delivered == 0 { + // callErr is a syscall.Errno and is never nil, so log it as the last + // error rather than as proof anything went wrong. + slog.Warn("PATH was saved, but the environment change broadcast did not complete; new terminals still inherit it", "last_error", callErr) + } +} diff --git a/services/nvpair-engine-manager/userpath_windows_entries.go b/services/nvpair-engine-manager/userpath_windows_entries.go new file mode 100644 index 00000000..1138459f --- /dev/null +++ b/services/nvpair-engine-manager/userpath_windows_entries.go @@ -0,0 +1,80 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +package main + +import "strings" + +// Windows PATH string algebra: splitting HKCU\Environment\Path on ';' and +// deciding what one entry means. Deliberately untagged, like userpath_shell.go, +// so the parsing rules are testable on every platform rather than only on the +// one CI happens to run. userpath_windows.go holds the registry access these +// feed, and that file is the Windows-only half. + +// Remove only the entry we wrote; differently spelled or expanded user entries +// are preserved. Comparison matches the one appendWindowsPath used to decide +// the entry was missing, so an external tool that re-cased the entry or gave it +// a trailing separator cannot make it unremovable. +// +// Unlike appendWindowsPath, no expansion is applied: the receipt records the +// literal entry PAIR wrote, and expanding a user's %VAR% here could match an +// entry PAIR never added. +// +// The search runs from the end because appendWindowsPath appends. The receipt +// records a directory, not a position, so when something else has since added +// the same directory the last match is the better guess at PAIR's own — and +// installers conventionally prepend. Exactly one goes, leaving whatever the +// other tool wanted where it wanted it. +func removeWindowsPath(current, owned string) string { + if owned == "" { + return current + } + entries := strings.Split(current, ";") + for i := len(entries) - 1; i >= 0; i-- { + if strings.EqualFold(normalizeWindowsPathEntry(entries[i]), normalizeWindowsPathEntry(owned)) { + return strings.Join(append(entries[:i:i], entries[i+1:]...), ";") + } + } + return current +} + +// addWindowsPath records newly appended entries but never claims existing ones. +// Ownership is recorded only when the PATH actually changed, which is the +// Windows half of the rule stated in userpath.go. +func addWindowsPath(current, dir string, expand func(string) string, receipt *pathReceipt, save func() error) (string, error) { + next := appendWindowsPath(current, dir, expand) + if next != current { + receipt.WindowsEntry = dir + if err := save(); err != nil { + return current, err + } + } + return next, nil +} + +// appendWindowsPath preserves existing entries and their order, including +// expandable environment references. Windows directory comparisons ignore case. +// +// A trailing separator is dropped rather than carried through: appending after +// one turns a harmless trailing empty element into an interior one, and Windows +// executable and DLL search has historically resolved an empty PATH element +// against the current directory. +func appendWindowsPath(current, dir string, expand func(string) string) string { + for _, entry := range strings.Split(current, ";") { + if strings.EqualFold(normalizeWindowsPathEntry(expand(entry)), normalizeWindowsPathEntry(dir)) { + return current + } + } + current = strings.TrimRight(current, ";") + if current == "" { + return dir + } + return current + ";" + dir +} + +// normalizeWindowsPathEntry makes two spellings of one directory comparable: +// surrounding quotes and spaces, slash direction, and a trailing separator are +// all cosmetic on Windows. +func normalizeWindowsPathEntry(value string) string { + return strings.TrimRight(strings.ReplaceAll(strings.Trim(value, " \""), "/", `\`), `\`) +} diff --git a/services/nvpair-tui/ui/engines.go b/services/nvpair-tui/ui/engines.go index 809febfb..53a2e5a7 100644 --- a/services/nvpair-tui/ui/engines.go +++ b/services/nvpair-tui/ui/engines.go @@ -27,6 +27,13 @@ type engineStatus struct { Running bool `json:"running"` Healthy bool `json:"healthy"` Port int `json:"port"` + PathManaged bool `json:"path_managed"` +} + +// pathPrompt is a lifecycle op held back until the user answers whether it may +// change their PATH. +type pathPrompt struct { + method, what, engine, question string } // enginesView manages local inference engines via the engine-manager @@ -43,6 +50,7 @@ type enginesView struct { input textinput.Model pulling bool pullEngine string + confirm *pathPrompt width, height int } @@ -181,9 +189,23 @@ func (v *enginesView) Update(msg tea.Msg) tea.Cmd { return nil } -func (v *enginesView) CapturingInput() bool { return v.pulling } +func (v *enginesView) CapturingInput() bool { return v.pulling || v.confirm != nil } func (v *enginesView) handleKey(msg tea.KeyMsg) tea.Cmd { + if p := v.confirm; p != nil { + switch msg.String() { + case "y", "Y": + v.confirm = nil + return v.runLifecycle(p.method, p.what, p.engine, lifecycleParams(p.engine, true)) + case "n", "N": + v.confirm = nil + return v.runLifecycle(p.method, p.what, p.engine, lifecycleParams(p.engine, false)) + case "esc": + v.confirm = nil + v.status = p.what + " " + p.engine + " cancelled" + } + return nil + } if v.pulling { switch msg.String() { case "enter": @@ -270,10 +292,48 @@ func (v *enginesView) handleAction(msg tea.KeyMsg) (tea.Cmd, bool) { if engine == "" { return nil, true } + if question := pathQuestion(method, v.byName[engine]); question != "" { + v.confirm = &pathPrompt{method: method, what: what, engine: engine, question: question} + return nil, true + } + return v.runLifecycle(method, what, engine, unaskedParams(engine)), true +} + +// unaskedParams carries no PATH answer because none was asked for, so an +// uninstall keeps PAIR's claim on any entry instead of handing it over unasked. +func unaskedParams(engine string) map[string]any { + return map[string]any{"engine": engine} +} + +// pathQuestion is what to ask before a lifecycle op may change the user's +// PATH, or "" when the op leaves it alone. An install always asks; an uninstall +// asks only when PAIR owns an entry to remove. +func pathQuestion(method string, e engineStatus) string { + label := e.Engine + if e.DisplayName != "" { + label = e.DisplayName + } + switch { + case method == "engine:install": + return fmt.Sprintf("Add the %s command-line tools to your PATH? Open a new terminal afterward. (y/n, esc cancels)", label) + case method == "engine:uninstall" && e.PathManaged: + return fmt.Sprintf("Also remove %s from your PATH? (y/n, esc cancels)", label) + } + return "" +} + +// lifecycleParams carries the user's PATH answer. The engine-manager reads a +// false path as "leave PATH alone" on install and "keep the entries as mine" on +// uninstall. +func lifecycleParams(engine string, path bool) map[string]any { + return map[string]any{"engine": engine, "path": path} +} + +func (v *enginesView) runLifecycle(method, what, engine string, params map[string]any) tea.Cmd { v.status = what + " " + engine + "..." - return call(v.client, method, map[string]string{"engine": engine}, func(_ *rpc.Message, err error) tea.Msg { + return call(v.client, method, params, func(_ *rpc.Message, err error) tea.Msg { return engineOpMsg{what: what, engine: engine, err: err} - }), true + }) } func (v *enginesView) selectedEngine() string { @@ -326,6 +386,9 @@ func (v *enginesView) View() string { if v.pulling { out += "\npull model: " + v.input.View() } + if v.confirm != nil { + out += "\n" + v.confirm.question + } if v.status != "" { out += "\n" + footerStyle.Render(v.status) } diff --git a/services/nvpair-tui/ui/engines_test.go b/services/nvpair-tui/ui/engines_test.go index e04a2c77..0ab58e20 100644 --- a/services/nvpair-tui/ui/engines_test.go +++ b/services/nvpair-tui/ui/engines_test.go @@ -3,7 +3,72 @@ package ui -import "testing" +import ( + "strings" + "testing" + + tea "github.com/charmbracelet/bubbletea" +) + +func runeKey(r string) tea.KeyMsg { return tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune(r)} } + +// Install always asks before touching PATH; uninstall asks only when PAIR owns +// an entry to remove, so there is never a question with nothing behind it. +func TestLifecycleAsksBeforeChangingPath(t *testing.T) { + view := func(e engineStatus) *enginesView { + v := newEnginesView(nil) + v.SetSize(80, 20) + v.merge(e) + return v + } + t.Run("install waits for an answer", func(t *testing.T) { + v := view(engineStatus{Engine: "ollama", DisplayName: "Ollama"}) + if cmd := v.handleKey(runeKey("i")); cmd != nil { + t.Fatal("install ran before the PATH question was answered") + } + if v.confirm == nil || !strings.Contains(v.confirm.question, "Ollama") || !v.CapturingInput() { + t.Fatalf("confirm = %+v, want a PATH question that captures input", v.confirm) + } + if cmd := v.handleKey(runeKey("y")); cmd == nil { + t.Fatal("answering yes did not run the install") + } + if v.confirm != nil { + t.Fatal("the question is still showing after the answer") + } + }) + t.Run("esc cancels the install", func(t *testing.T) { + v := view(engineStatus{Engine: "ollama"}) + v.handleKey(runeKey("i")) + if cmd := v.handleKey(tea.KeyMsg{Type: tea.KeyEsc}); cmd != nil { + t.Fatal("esc still ran the install") + } + if v.confirm != nil || !strings.Contains(v.status, "cancelled") { + t.Fatalf("confirm = %+v, status = %q", v.confirm, v.status) + } + }) + t.Run("uninstall asks only when PAIR owns the entry", func(t *testing.T) { + managed := view(engineStatus{Engine: "lmstudio", Installed: true, PathManaged: true}) + if cmd := managed.handleKey(runeKey("u")); cmd != nil || managed.confirm == nil { + t.Fatal("uninstall of a PAIR-owned PATH entry did not ask first") + } + unmanaged := view(engineStatus{Engine: "lmstudio", Installed: true}) + if cmd := unmanaged.handleKey(runeKey("u")); cmd == nil || unmanaged.confirm != nil { + t.Fatal("uninstall asked about a PATH entry PAIR never added") + } + }) +} + +func TestLifecycleParamsCarryThePathAnswer(t *testing.T) { + for _, answer := range []bool{true, false} { + p := lifecycleParams("ollama", answer) + if p["engine"] != "ollama" || p["path"] != answer { + t.Errorf("lifecycleParams(ollama, %v) = %v", answer, p) + } + } + if _, ok := unaskedParams("ollama")["path"]; ok { + t.Error("an op the user was not asked about sent a PATH answer") + } +} // TestPullParamsSendsBothKeys guards the LM Studio pull fix: the pull params // must carry the model under BOTH "name" (Ollama's /api/pull body key) and