From a1b3de5de589bd7c3696ed56ae04df5348b901c7 Mon Sep 17 00:00:00 2001 From: Chris Kelsey Date: Tue, 22 Sep 2026 15:47:40 -0700 Subject: [PATCH 1/4] feat(engine-manager): publish installed engine CLIs on the user's PATH When PAIR installs Ollama or LM Studio, add that engine's command-line directory to the installing user's persistent PATH on that device, and give back only what PAIR recorded when the engine or the application is removed. Open a new terminal after either operation. Local installs only. An install driven by a cluster peer deliberately skips the PATH step, because rewriting the login shell of whoever is sitting at the target node is not a paired peer's decision. Ownership is recorded outside the engine directory and before the PATH is touched, so a crash mid-update is retryable and an uninstall can delete exactly what PAIR added. Existing entries, user-edited shell snippets, and engines the user installed themselves are preserved. The record also notes that PAIR ran the installer, which is what lets a reinstall re-adopt an engine whose vendor owns its location: LM Studio writes ~/.lmstudio, so the path is no evidence of who put it there. A record that exists but cannot be parsed fails an uninstall rather than reporting a clean one. Windows persists through HKCU\Environment; the other platforms append a self-identifying block to the login shell's profiles. Both run under a cross-process lock that covers the whole read-modify-write, bounded so a hung peer times out instead of wedging the operation, and both publish through a temporary file and an atomic rename. A profile rewrite re-checks the file's fingerprint before renaming, so a concurrent editor's save is refused rather than silently discarded. The platform uninstallers release the entries before deleting the data directory that holds the records, and keep that data when the release fails so a reinstall can finish the cleanup. Two changes are adaptations to this tree rather than part of the feature. runCommand takes a variadic environment for install commands that declare overrides, so the launch closure in lifecycle.go accepts and ignores it; a launch carries its own environment separately. writeJSONAtomic keeps this tree's unique temporary name and gains the file flush and directory flush that the ownership records need, since a rename alone says nothing about bytes reaching stable storage. Signed-off-by: Chris Kelsey --- desktop/electron-builder.config.ts | 11 +- desktop/scripts/build/installer.nsh | 50 +- desktop/scripts/build/linux/before-remove.sh | 61 ++ desktop/scripts/build/macos/uninstall.sh | 39 + docs/engine-lifecycle.mdx | 17 + docs/getting-started.mdx | 60 +- docs/known-issues.mdx | 7 - scripts/wipe-app-data.ps1 | 10 + scripts/wipe-app-data.sh | 10 + services/nvpair-engine-manager/MANIFEST.md | 15 + services/nvpair-engine-manager/README.md | 49 +- .../nvpair-engine-manager/controlstream.go | 4 +- services/nvpair-engine-manager/e2e_test.go | 21 +- services/nvpair-engine-manager/executor.go | 9 + .../nvpair-engine-manager/executor_test.go | 6 +- services/nvpair-engine-manager/install.go | 155 +++- .../install_path_test.go | 352 +++++++++ services/nvpair-engine-manager/lifecycle.go | 6 +- services/nvpair-engine-manager/live_test.go | 37 +- services/nvpair-engine-manager/main.go | 24 + .../manifests/lmstudio.json | 1 + .../nvpair-engine-manager/pathlock_unix.go | 28 + .../nvpair-engine-manager/pathlock_windows.go | 32 + .../nvpair-engine-manager/pathownership.go | 312 ++++++++ .../pathownership_test.go | 668 ++++++++++++++++++ services/nvpair-engine-manager/registry.go | 26 + services/nvpair-engine-manager/setport.go | 32 +- services/nvpair-engine-manager/spec.md | 23 +- .../nvpair-engine-manager/syncdir_unix.go | 37 + .../nvpair-engine-manager/syncdir_windows.go | 8 + .../testdata/fakeengine/main.go | 27 +- services/nvpair-engine-manager/userpath.go | 58 ++ .../nvpair-engine-manager/userpath_shell.go | 231 ++++++ .../nvpair-engine-manager/userpath_test.go | 238 +++++++ .../nvpair-engine-manager/userpath_unix.go | 222 ++++++ .../userpath_unix_test.go | 79 +++ .../nvpair-engine-manager/userpath_windows.go | 103 +++ .../userpath_windows_entries.go | 80 +++ 38 files changed, 3063 insertions(+), 85 deletions(-) create mode 100644 desktop/scripts/build/linux/before-remove.sh create mode 100644 services/nvpair-engine-manager/install_path_test.go create mode 100644 services/nvpair-engine-manager/pathlock_unix.go create mode 100644 services/nvpair-engine-manager/pathlock_windows.go create mode 100644 services/nvpair-engine-manager/pathownership.go create mode 100644 services/nvpair-engine-manager/pathownership_test.go create mode 100644 services/nvpair-engine-manager/syncdir_unix.go create mode 100644 services/nvpair-engine-manager/syncdir_windows.go create mode 100644 services/nvpair-engine-manager/userpath.go create mode 100644 services/nvpair-engine-manager/userpath_shell.go create mode 100644 services/nvpair-engine-manager/userpath_test.go create mode 100644 services/nvpair-engine-manager/userpath_unix.go create mode 100644 services/nvpair-engine-manager/userpath_unix_test.go create mode 100644 services/nvpair-engine-manager/userpath_windows.go create mode 100644 services/nvpair-engine-manager/userpath_windows_entries.go diff --git a/desktop/electron-builder.config.ts b/desktop/electron-builder.config.ts index 1f713d47..612bb7be 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 has to be prerm rather than postrm because dpkg deletes the package's + // files before postrm runs, and the PATH cleanup runs a binary from /opt — + // 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/before-remove.sh b/desktop/scripts/build/linux/before-remove.sh new file mode 100644 index 00000000..cfbfaa1c --- /dev/null +++ b/desktop/scripts/build/linux/before-remove.sh @@ -0,0 +1,61 @@ +#!/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. Releases the PATH entries this user's +# engines own. 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 prerm AND NOT postrm. 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. +# after-remove handles the data root on `purge` -- but dpkg deletes the +# package's files before postrm runs, so by then the binary that understands +# those records is gone. prerm is the last point at which both still exist. +# +# This runs on `apt remove` as well as `apt purge`, so a plain remove gives up +# its PATH entries even though it keeps the engines. That is the recoverable +# direction: the drain releases each entry but keeps the record's note that PAIR +# installed the engine, so a reinstall re-adopts it and republishes. Deleting +# the record outright is what made this unrecoverable for an engine whose CLI +# lives outside PAIR's install directory -- nothing else tells it apart from an +# engine the user installed. The alternative -- entries no tool can identify, +# pointing into a directory a later purge deletes -- is worse either way. +# +# dpkg calls prerm with "upgrade" during an update, and "failed-upgrade" when +# recovering from one. Those keep the installation, so leave PATH alone. +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. +command -v dpkg-query >/dev/null 2>&1 || exit 0 +engine_manager="$(dpkg-query -L "${DPKG_MAINTSCRIPT_PACKAGE:-}" 2>/dev/null \ + | grep -E '/cli-bin/nvpair-engine-manager$' | head -n 1)" +[ -n "$engine_manager" ] && [ -x "$engine_manager" ] || exit 0 + +# prerm runs as root, while the records, the dotfiles, and $XDG_CONFIG_HOME all +# belong to the user who ran the app. Resolve that user the same way +# after-remove resolves the data root on purge, and run as them so the binary +# reads the environment it wrote under. Best-effort: on a multi-user box, other +# users' entries are left for their own reinstall to reclaim. +real_user="${SUDO_USER:-}" +if [ -z "$real_user" ] && command -v logname >/dev/null 2>&1; then + real_user="$(logname 2>/dev/null || true)" +fi +[ -n "$real_user" ] && [ "$real_user" != root ] || exit 0 + +if command -v runuser >/dev/null 2>&1; then + runuser -u "$real_user" -- "$engine_manager" --remove-user-path >/dev/null 2>&1 || true +else + su -s /bin/sh -c "'$engine_manager' --remove-user-path" "$real_user" >/dev/null 2>&1 || true +fi + +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/docs/engine-lifecycle.mdx b/docs/engine-lifecycle.mdx index 0d492277..8131a89f 100644 --- a/docs/engine-lifecycle.mdx +++ b/docs/engine-lifecycle.mdx @@ -70,6 +70,23 @@ Refer also to [Getting started](getting-started.mdx). You can install a supported engine during first-run setup or later from **Engine settings**. +PAIR automatically adds the installed engine's command-line tools (`ollama` or +`lms`) to your PATH. 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. + +On uninstall, PAIR removes only the PATH entries it recorded adding for an +engine it installed. Pre-existing entries and user-edited shell blocks are +preserved. Installations without an ownership record are left unchanged. +Uninstalling PAIR itself releases any entries its engines still own. + 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..e975025a 100644 --- a/docs/getting-started.mdx +++ b/docs/getting-started.mdx @@ -351,51 +351,59 @@ 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 adds that engine's command directory to your +user `PATH`, so `ollama` or `lms` works by name. **Open a new terminal +afterwards** — a shell that was already running keeps the `PATH` it started with. +Uninstalling the engine removes the entry again. If PAIR could not update your +`PATH`, it tells you so and 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 `PATH` setup 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..9dc35fb5 100644 --- a/services/nvpair-engine-manager/README.md +++ b/services/nvpair-engine-manager/README.md @@ -53,6 +53,36 @@ Requests (caller → service): `EngineStatus` = `{ engine, display_name, installed, running, healthy, port }`. +A locally-initiated install publishes the engine's CLI directory on this user's +PATH. 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 — skips this +step entirely. 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 PAIR removes only recorded +entries or unchanged shell snippets; pre-existing entries and unrecorded +installations are preserved. Cleanup failures keep the receipt for a later +uninstall retry, even after the executable is gone. + +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. + +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}`, `engine:models-changed{engine, models}` — pushed when an engine's set of @@ -238,6 +268,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 +317,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/controlstream.go b/services/nvpair-engine-manager/controlstream.go index 13bd584d..f4b554dd 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 { 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..fd444e9a 100644 --- a/services/nvpair-engine-manager/executor.go +++ b/services/nvpair-engine-manager/executor.go @@ -93,6 +93,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 +127,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 +277,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..c719a9ff 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,6 +406,7 @@ 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 { t.Fatalf("install should adopt the external service: %v", err) diff --git a/services/nvpair-engine-manager/install.go b/services/nvpair-engine-manager/install.go index 268bd0b9..d38d0a5c 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,31 @@ 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. +// Install obtains the engine in user mode for a request that originated on this +// machine: download (checksum-verified), run the declared command, then publish +// the CLI directory on this user's PATH. func (e *Executor) Install(ctx context.Context, engine string) error { + return e.install(ctx, engine, true) +} + +// 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. +func (e *Executor) InstallForPeer(ctx context.Context, engine string) error { + return e.install(ctx, engine, false) +} + +func (e *Executor) install(ctx context.Context, engine string, setUserPath bool) error { st, err := e.state(engine) if err != nil { return err @@ -29,6 +48,9 @@ func (e *Executor) Install(ctx context.Context, engine string) error { st.opMu.Lock() defer st.opMu.Unlock() if ok, _ := e.Detect(engine); ok { + if setUserPath { + e.installPath(engine, st, false) + } e.reporter.clear(installFailedID(engine)) e.emitInstallProgress(engine, "already-installed", 100) return nil @@ -65,6 +87,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 +102,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 +129,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,12 +142,92 @@ func (e *Executor) Install(ctx context.Context, engine string) error { e.reportInstallFailed(engine, err) return err } + if setUserPath { + e.installPath(engine, st, true) + } 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 { @@ -135,7 +238,10 @@ 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. + return errors.Join(e.setDesiredEnabled(engine, false), e.uninstallPath(engine)) } un := st.plat.Uninstall if un == nil || len(un.Run) == 0 { @@ -204,9 +310,11 @@ func (e *Executor) Uninstall(ctx context.Context, engine string) error { st.mu.Lock() st.binPath = "" st.mu.Unlock() - e.reporter.clear(uninstallFailedID(engine)) e.emitState(engine) - return e.setDesiredEnabled(engine, false) + // 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. + return errors.Join(e.setDesiredEnabled(engine, false), e.uninstallPath(engine)) } // maxDownloadBytes caps a single engine download (engine installers / @@ -309,11 +417,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 +433,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..24171b1c --- /dev/null +++ b/services/nvpair-engine-manager/install_path_test.go @@ -0,0 +1,352 @@ +// 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" + "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 TestInstallAutomaticallyAddsCLIToPath(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); 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"); 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"); 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"); 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"); 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"); 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"); 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"); 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"); err == nil { + t.Fatal("expected install failure") + } +} + +func TestInstallRPCAutomaticallyAddsCLIToPath(t *testing.T) { + ex, cli := pathInstallExecutor(t, "ollama", "process") + var added string + ex.addToPath = func(dir string, _ *pathReceipt, _ func() error) error { added = dir; return nil } + 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: "engine:install", + Params: json.RawMessage(`{"engine":"ollama"}`)}) + 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("install returned an error: %s", *response.Error) + } + if !response.Result.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)) + } +} + +// 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 _, err := os.Stat(ex.pathReceiptFile("lmstudio")); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("remote install claimed PATH ownership: %v", err) + } +} 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..c952f2b4 100644 --- a/services/nvpair-engine-manager/live_test.go +++ b/services/nvpair-engine-manager/live_test.go @@ -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/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..d97a4c4f --- /dev/null +++ b/services/nvpair-engine-manager/pathownership.go @@ -0,0 +1,312 @@ +// 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, by + // deleting the whole record. + // + // 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"` +} + +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) + if err == nil { + err = os.Remove(file) + } + } + 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 +} + +// 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..befa7377 --- /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); 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); 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"); err != nil { + t.Fatal(err) + } + if err := ex.Uninstall(context.Background(), "lmstudio"); 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"); 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"); 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"); 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"); 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"); 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"); 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"); 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"); 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"); 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"); 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"); 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"); 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"); 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") + 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"); 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"); 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"); 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"); 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"); 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"); 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"); !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"); 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/setport.go b/services/nvpair-engine-manager/setport.go index 66f0b283..8cfeb945 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) + // 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 := os.Rename(tmp, path); err != nil { - _ = os.Remove(tmp) - return fmt.Errorf("rename override: %w", err) + return fmt.Errorf("rename %s: %w", name, err) } - return nil + return syncDir(filepath.Dir(path)) } diff --git a/services/nvpair-engine-manager/spec.md b/services/nvpair-engine-manager/spec.md index ba2e6324..8afe3e92 100644 --- a/services/nvpair-engine-manager/spec.md +++ b/services/nvpair-engine-manager/spec.md @@ -33,11 +33,14 @@ 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 publishes the engine's CLI directory on this user's PATH. +- **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. 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. @@ -112,8 +115,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 }` | `EngineStatus` (after install and PATH publication) | +| `engine:uninstall` | `{ engine }` | `EngineStatus` (after removal and PATH cleanup) | | `engine:start` | `{ engine }` | `EngineStatus` (after readiness) | | `engine:stop` | `{ engine }` | `EngineStatus` | | `engine:restart` | `{ engine }` | `EngineStatus` | @@ -158,14 +161,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 deletes the record. The application uninstaller (`--remove-user-path`) 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 the 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 +181,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 and treated as no claim; 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/syncdir_unix.go b/services/nvpair-engine-manager/syncdir_unix.go new file mode 100644 index 00000000..74c257c2 --- /dev/null +++ b/services/nvpair-engine-manager/syncdir_unix.go @@ -0,0 +1,37 @@ +// 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()) +} diff --git a/services/nvpair-engine-manager/syncdir_windows.go b/services/nvpair-engine-manager/syncdir_windows.go new file mode 100644 index 00000000..906c86b0 --- /dev/null +++ b/services/nvpair-engine-manager/syncdir_windows.go @@ -0,0 +1,8 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +package main + +// 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 } 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, " \""), "/", `\`), `\`) +} From ab7a88f1c212f6dea8f88e6043368656734d0dab Mon Sep 17 00:00:00 2001 From: Chris Kelsey Date: Mon, 28 Sep 2026 15:11:42 -0700 Subject: [PATCH 2/4] feat: ask before adding or removing engines on the user's PATH PATH changes are now the user's decision. engine:install and engine:uninstall take a path flag, and an absent or false flag leaves PATH untouched, so a client that does not ask changes nothing. EngineStatus reports path_managed so a client offers removal only when PAIR owns an entry. A declined install still records that PAIR ran the installer, so a later consented install can re-adopt an engine whose vendor owns its location. A declined uninstall leaves the entries and drops the record, handing them to the user. The desktop app asks with a yes/no modal before a local install, once for every engine the first-run wizard installs, and adds an "also remove from my PATH" checkbox to the uninstall confirmation when PAIR owns the entry. Update carries the existing answer through without asking. The TUI asks the same questions with y/n prompts. Signed-off-by: Chris Kelsey --- desktop/docs/frontend-api.md | 6 + desktop/docs/services-api.md | 2 + desktop/docs/services-parity.md | 3 +- .../electron/service-bridge/empty-handlers.ts | 14 +- .../electron/service-bridge/modular-state.ts | 16 +- desktop/src/shared/types/engine-api.ts | 6 + desktop/src/shared/types/engines.ts | 6 + desktop/src/ui/api/engine-api.ts | 19 ++- .../ui/components/BackendRow/BackendRow.tsx | 41 ++++- desktop/src/ui/components/ConfirmModal.tsx | 19 +++ .../ui/components/EnginePathConsentModal.tsx | 72 +++++++++ desktop/src/ui/components/ErrorModal.tsx | 120 ++++++++++++-- .../components/NodeList/NodeEnginesInline.tsx | 25 ++- .../ui/components/Welcome/WelcomeModal.tsx | 33 +++- .../tests/modular/engine-path-consent.test.ts | 91 +++++++++++ .../modular/engine-path-managed-state.test.ts | 54 +++++++ docs/engine-lifecycle.mdx | 19 ++- docs/getting-started.mdx | 13 +- services/nvpair-engine-manager/README.md | 29 ++-- services/nvpair-engine-manager/executor.go | 3 + .../nvpair-engine-manager/executor_test.go | 24 +-- services/nvpair-engine-manager/install.go | 57 +++++-- .../install_path_test.go | 153 +++++++++++++++--- services/nvpair-engine-manager/live_test.go | 8 +- services/nvpair-engine-manager/manager.go | 9 +- .../nvpair-engine-manager/pathownership.go | 52 ++++++ .../pathownership_test.go | 52 +++--- .../nvpair-engine-manager/remediation_test.go | 4 +- services/nvpair-engine-manager/spec.md | 12 +- services/nvpair-engine-manager/status.go | 2 + services/nvpair-tui/ui/engines.go | 62 ++++++- services/nvpair-tui/ui/engines_test.go | 64 +++++++- 32 files changed, 933 insertions(+), 157 deletions(-) create mode 100644 desktop/src/ui/components/EnginePathConsentModal.tsx create mode 100644 desktop/tests/modular/engine-path-consent.test.ts create mode 100644 desktop/tests/modular/engine-path-managed-state.test.ts diff --git a/desktop/docs/frontend-api.md b/desktop/docs/frontend-api.md index 63037d09..86ed8374 100644 --- a/desktop/docs/frontend-api.md +++ b/desktop/docs/frontend-api.md @@ -117,6 +117,12 @@ environment assignments pass through without an engine-option catalog. - `deleteModel`; - `setModelExpiry`. +`install` and `uninstall` carry `path`, the user's answer to changing their +PATH. Absent means no consent. 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`. Remote installs never change +PATH, and `update` reuses the existing `pathManaged` 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/src/electron/service-bridge/empty-handlers.ts b/desktop/src/electron/service-bridge/empty-handlers.ts index 14fd3d89..730958b7 100644 --- a/desktop/src/electron/service-bridge/empty-handlers.ts +++ b/desktop/src/electron/service-bridge/empty-handlers.ts @@ -203,7 +203,7 @@ 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 ) @@ -213,32 +213,36 @@ function routeEngineManagerCommand(payload: WsInvokeRequest<'engine:command'>): supervisor.sendProcess( 'broker', 'engine:uninstall', - { engine }, + { engine, path: payload.path === true }, failPendingOp('uninstall', 'uninstall'), true ) break - case 'update': + 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`. + // An update carries the user's earlier PATH answer through both + // steps rather than asking again. + const path = state.localEnginePathManaged(payload.engineType) state.beginLocalEngineOp(payload.engineType, 'installing') supervisor.sendProcess( 'broker', 'engine:uninstall', - { engine }, + { engine, path }, failPendingOp('update', 'uninstall'), true ) supervisor.sendProcess( 'broker', 'engine:install', - { engine, start: true }, + { engine, start: true, path }, failPendingOp('update', 'install'), true ) break + } case 'toggle': void toggleLocalEngine(engine, payload.engineType) break 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..68bffa80 100644 --- a/desktop/src/shared/types/engine-api.ts +++ b/desktop/src/shared/types/engine-api.ts @@ -57,6 +57,12 @@ 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). Absent means + * no consent, so PATH is left as it is. + */ + 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..95f78518 100644 --- a/desktop/src/ui/api/engine-api.ts +++ b/desktop/src/ui/api/engine-api.ts @@ -48,10 +48,13 @@ 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, + * removing the PATH entry PAIR added when the user agreed. + */ + 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 +97,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..c6cd1dc6 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) + }, [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..04e6ca1d 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,93 @@ 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(true) + + // 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) { + setRemovePath(true) + setAskUninstallPath(true) + } else { + rerun(() => window.pairApi.engines.uninstall(engineType, nodeId, false)) + } + } 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..6297eb02 --- /dev/null +++ b/desktop/tests/modular/engine-path-consent.test.ts @@ -0,0 +1,91 @@ +// 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('treats an omitted answer as no consent', async () => { + await handleServiceBridgeInvoke('engine:command', { + command: 'uninstall', + engineType: 'ollama', + nodeId: 'local-node' + }) + + expect(sentParams('engine:uninstall')).toEqual([{ engine: 'ollama', path: false }]) + }) + + it('carries the existing PATH choice through both steps of an update', async () => { + mocks.state.localEnginePathManaged.mockReturnValue(true) + + await handleServiceBridgeInvoke('engine:command', { + command: 'update', + engineType: 'ollama', + nodeId: 'local-node' + }) + + expect(sentParams('engine:uninstall')).toEqual([{ engine: 'ollama', path: true }]) + expect(sentParams('engine:install')).toEqual([ + { engine: 'ollama', start: true, path: true } + ]) + }) +}) 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 8131a89f..7a24a819 100644 --- a/docs/engine-lifecycle.mdx +++ b/docs/engine-lifecycle.mdx @@ -70,9 +70,12 @@ Refer also to [Getting started](getting-started.mdx). You can install a supported engine during first-run setup or later from **Engine settings**. -PAIR automatically adds the installed engine's command-line tools (`ollama` or -`lms`) to your PATH. Open a new terminal after installation; on Windows, you may -need to restart your terminal app. +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 @@ -82,10 +85,12 @@ 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. -On uninstall, PAIR removes only the PATH entries it recorded adding for an -engine it installed. Pre-existing entries and user-edited shell blocks are -preserved. Installations without an ownership record are left unchanged. -Uninstalling PAIR itself releases any entries its engines still own. +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 you install an engine, consider the following: diff --git a/docs/getting-started.mdx b/docs/getting-started.mdx index e975025a..2fe81a3f 100644 --- a/docs/getting-started.mdx +++ b/docs/getting-started.mdx @@ -353,11 +353,12 @@ current URL. An engine PAIR installed for you is a normal installation, and you can drive it with its own command-line interface (CLI). -When PAIR installs an engine, it adds that engine's command directory to your -user `PATH`, so `ollama` or `lms` works by name. **Open a new terminal -afterwards** — a shell that was already running keeps the `PATH` it started with. -Uninstalling the engine removes the entry again. If PAIR could not update your -`PATH`, it tells you so and the engine still works; use the full path below. +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. @@ -383,7 +384,7 @@ official installer: lms status ``` -If you need the full path — because `PATH` setup failed, or you are scripting +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 | diff --git a/services/nvpair-engine-manager/README.md b/services/nvpair-engine-manager/README.md index 9dc35fb5..ceaf8900 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,10 +51,19 @@ 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 for the engine on this +machine, so a client asks about removing one only when there is one. -A locally-initiated install publishes the engine's CLI directory on this user's -PATH. Windows updates `HKCU\Environment\Path`; Unix appends a block to the login +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. 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. @@ -71,10 +80,12 @@ 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 PAIR removes only recorded -entries or unchanged shell snippets; pre-existing entries and unrecorded -installations are preserved. Cleanup failures keep the receipt for a later -uninstall retry, even after the executable is gone. +cannot clobber it. After a successful uninstall with `path:true` PAIR removes +only recorded entries or unchanged shell snippets; 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 without +`path:true` leaves the entries in place and deletes the receipt, so they become +the user's and `--remove-user-path` no longer touches them. 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 diff --git a/services/nvpair-engine-manager/executor.go b/services/nvpair-engine-manager/executor.go index fd444e9a..0e63907d 100644 --- a/services/nvpair-engine-manager/executor.go +++ b/services/nvpair-engine-manager/executor.go @@ -35,6 +35,9 @@ 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. + PathManaged bool `json:"path_managed"` } // engineState is the per-engine runtime state. diff --git a/services/nvpair-engine-manager/executor_test.go b/services/nvpair-engine-manager/executor_test.go index c719a9ff..79aa0db7 100644 --- a/services/nvpair-engine-manager/executor_test.go +++ b/services/nvpair-engine-manager/executor_test.go @@ -408,7 +408,7 @@ func TestInstallAdoptsExternalServiceWithoutDownloading(t *testing.T) { 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 { @@ -460,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) } @@ -502,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) { @@ -609,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") { @@ -666,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) } } @@ -706,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 { @@ -753,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) } @@ -776,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) } @@ -810,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) } @@ -888,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) } @@ -917,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 { @@ -928,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 d38d0a5c..d22d0823 100644 --- a/services/nvpair-engine-manager/install.go +++ b/services/nvpair-engine-manager/install.go @@ -21,11 +21,28 @@ import ( "time" ) +// installPathMode says what an install may do to the user's PATH. +type installPathMode int + +const ( + // pathUntouched leaves PATH and the ownership record alone. + pathUntouched installPathMode = iota + // 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 + // pathPublish publishes the CLI directory on the user's PATH. + pathPublish +) + // Install obtains the engine in user mode for a request that originated on this -// machine: download (checksum-verified), run the declared command, then publish -// the CLI directory on this user's PATH. -func (e *Executor) Install(ctx context.Context, engine string) error { - return e.install(ctx, engine, true) +// 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. @@ -37,10 +54,10 @@ func (e *Executor) Install(ctx context.Context, engine string) error { // nothing in the remote-install flow tells the person sitting at this machine // that their shell startup files are about to be rewritten. func (e *Executor) InstallForPeer(ctx context.Context, engine string) error { - return e.install(ctx, engine, false) + return e.install(ctx, engine, pathUntouched) } -func (e *Executor) install(ctx context.Context, engine string, setUserPath bool) error { +func (e *Executor) install(ctx context.Context, engine string, pathMode installPathMode) error { st, err := e.state(engine) if err != nil { return err @@ -48,7 +65,9 @@ func (e *Executor) install(ctx context.Context, engine string, setUserPath bool) st.opMu.Lock() defer st.opMu.Unlock() if ok, _ := e.Detect(engine); ok { - if setUserPath { + // 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)) @@ -142,8 +161,13 @@ func (e *Executor) install(ctx context.Context, engine string, setUserPath bool) e.reportInstallFailed(engine, err) return err } - if setUserPath { + 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) @@ -230,7 +254,11 @@ func managedCLI(st *engineState) bool { // 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 { st, err := e.state(engine) if err != nil { return err @@ -241,7 +269,7 @@ func (e *Executor) Uninstall(ctx context.Context, engine string) error { // 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. - return errors.Join(e.setDesiredEnabled(engine, false), e.uninstallPath(engine)) + return errors.Join(e.setDesiredEnabled(engine, false), e.finishUninstallPath(engine, removePath)) } un := st.plat.Uninstall if un == nil || len(un.Run) == 0 { @@ -314,7 +342,14 @@ func (e *Executor) Uninstall(ctx context.Context, engine string) error { // 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. - return errors.Join(e.setDesiredEnabled(engine, false), e.uninstallPath(engine)) + return errors.Join(e.setDesiredEnabled(engine, false), e.finishUninstallPath(engine, removePath)) +} + +func (e *Executor) finishUninstallPath(engine string, removePath bool) error { + if removePath { + return e.uninstallPath(engine) + } + return e.releasePathOwnership(engine) } // maxDownloadBytes caps a single engine download (engine installers / diff --git a/services/nvpair-engine-manager/install_path_test.go b/services/nvpair-engine-manager/install_path_test.go index 24171b1c..168e19ea 100644 --- a/services/nvpair-engine-manager/install_path_test.go +++ b/services/nvpair-engine-manager/install_path_test.go @@ -61,7 +61,7 @@ func pathWarning(ex *Executor, engine string) string { return "" } -func TestInstallAutomaticallyAddsCLIToPath(t *testing.T) { +func TestConsentedInstallAddsCLIToPath(t *testing.T) { test := func(name, engine, mode string) { t.Run(name, func(t *testing.T) { ex, cli := pathInstallExecutor(t, engine, mode) @@ -73,7 +73,7 @@ func TestInstallAutomaticallyAddsCLIToPath(t *testing.T) { added = append(added, dir) return nil } - if err := ex.Install(context.Background(), engine); err != nil { + if err := ex.Install(context.Background(), engine, true); err != nil { t.Fatal(err) } if !fileExists(cli) { @@ -108,7 +108,7 @@ func TestPathFailureStillCompletesInstallAndWarns(t *testing.T) { ex.addToPath = func(string, *pathReceipt, func() error) error { return errors.New("profile is read only") } - if err := ex.Install(context.Background(), "ollama"); err != nil { + 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") @@ -125,7 +125,7 @@ func TestPathFailureStillCompletesInstallAndWarns(t *testing.T) { var added string ex.addToPath = func(dir string, _ *pathReceipt, _ func() error) error { added = dir; return nil } - if err := ex.Install(context.Background(), "ollama"); err != nil { + if err := ex.Install(context.Background(), "ollama", true); err != nil { t.Fatal(err) } if added != filepath.Dir(cli) { @@ -147,7 +147,7 @@ func TestPathWarningOmitsTheUsersHomeDirectory(t *testing.T) { 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"); err != nil { + if err := ex.Install(context.Background(), "ollama", true); err != nil { t.Fatal(err) } warning := pathWarning(ex, "ollama") @@ -171,7 +171,7 @@ func TestPathRefusesAnUndeclaredCLIOutsideTheInstallDirectory(t *testing.T) { t.Error("claimed a PATH directory PAIR does not own") return nil } - if err := ex.Install(context.Background(), "ollama"); err != nil { + if err := ex.Install(context.Background(), "ollama", true); err != nil { t.Fatal(err) } if !fileExists(cli) { @@ -198,7 +198,7 @@ func TestPreexistingEngineProducesNoPathWarning(t *testing.T) { t.Error("claimed a PATH directory PAIR does not own") return nil } - if err := ex.Install(context.Background(), "ollama"); err != nil { + if err := ex.Install(context.Background(), "ollama", true); err != nil { t.Fatal(err) } if warning := pathWarning(ex, "ollama"); warning != "" { @@ -217,7 +217,7 @@ func TestPathRejectsARelativeManifestCLI(t *testing.T) { t.Error("published a PATH entry from a relative manifest path") return nil } - if err := ex.Install(context.Background(), "ollama"); err != nil { + if err := ex.Install(context.Background(), "ollama", true); err != nil { t.Fatal(err) } if warning := pathWarning(ex, "ollama"); !strings.Contains(warning, "relative path") { @@ -235,7 +235,7 @@ func TestInstallAppliesManifestDeclaredInstallerEnvironment(t *testing.T) { 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"); err != nil { + if err := ex.Install(context.Background(), "ollama", true); err != nil { t.Fatal(err) } value, err := os.ReadFile(record) @@ -282,20 +282,19 @@ func TestFailedInstallDoesNotChangePath(t *testing.T) { t.Error("PATH changed after failed install") return nil } - if err := ex.Install(context.Background(), "ollama"); err == nil { + if err := ex.Install(context.Background(), "ollama", true); err == nil { t.Fatal("expected install failure") } } -func TestInstallRPCAutomaticallyAddsCLIToPath(t *testing.T) { - ex, cli := pathInstallExecutor(t, "ollama", "process") - var added string - ex.addToPath = func(dir string, _ *pathReceipt, _ func() error) error { added = dir; return nil } +// 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: "engine:install", - Params: json.RawMessage(`{"engine":"ollama"}`)}) + 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"` @@ -304,16 +303,130 @@ func TestInstallRPCAutomaticallyAddsCLIToPath(t *testing.T) { t.Fatal(err) } if response.Error != nil { - t.Fatalf("install returned an error: %s", *response.Error) + t.Fatalf("%s returned an error: %s", method, *response.Error) } - if !response.Result.Installed { - t.Error("install reported the engine as not installed") + 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, _ *pathReceipt, _ func() error) error { added = dir; return nil } + 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.Errorf("published %q on PATH, want the CLI directory %q", 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("omitted path 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"}`) + 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 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) { diff --git a/services/nvpair-engine-manager/live_test.go b/services/nvpair-engine-manager/live_test.go index c952f2b4..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) diff --git a/services/nvpair-engine-manager/manager.go b/services/nvpair-engine-manager/manager.go index e12f62dc..a22969f3 100644 --- a/services/nvpair-engine-manager/manager.go +++ b/services/nvpair-engine-manager/manager.go @@ -35,11 +35,16 @@ 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 consent to change their PATH: on install, to publish the +// engine's CLI directory; on uninstall, to remove what PAIR published. It is +// absent unless a client asked the user, so an omitted field never touches it. 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 +312,11 @@ 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); err == nil && p.Start { err = start() } case "engine:uninstall": - err = m.exec.Uninstall(ctx, p.Engine) + err = m.exec.Uninstall(ctx, p.Engine, p.Path) case "engine:start": err = start() case "engine:stop": diff --git a/services/nvpair-engine-manager/pathownership.go b/services/nvpair-engine-manager/pathownership.go index d97a4c4f..d8b67fcc 100644 --- a/services/nvpair-engine-manager/pathownership.go +++ b/services/nvpair-engine-manager/pathownership.go @@ -251,6 +251,58 @@ func (e *Executor) uninstallPath(engine string) error { 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. +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 +} + +// 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. Records are published by +// atomic rename, so the read needs no lock. +func (e *Executor) pathManaged(engine string) bool { + receipt, err := loadPathReceipt(e.pathReceiptFile(engine)) + return err == nil && receipt.Dir != "" +} + // removeAllUserPaths releases every PATH entry the engines under baseDir own. // // The receipts live inside the data directory the application uninstaller diff --git a/services/nvpair-engine-manager/pathownership_test.go b/services/nvpair-engine-manager/pathownership_test.go index befa7377..5599772d 100644 --- a/services/nvpair-engine-manager/pathownership_test.go +++ b/services/nvpair-engine-manager/pathownership_test.go @@ -39,14 +39,14 @@ func TestUninstallRemovesOwnedPathAfterRestart(t *testing.T) { if err := os.WriteFile(profile, []byte(original), 0o644); err != nil { t.Fatal(err) } - if err := ex.Install(context.Background(), engine); err != nil { + 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); err != nil { + if err := restarted.Uninstall(context.Background(), engine, true); err != nil { t.Fatal(err) } data, err := os.ReadFile(profile) @@ -86,10 +86,10 @@ func TestDetectedExternalEngineDoesNotAcquirePathOwnership(t *testing.T) { t.Error("cleaned up an unowned PATH") return nil } - if err := ex.Install(context.Background(), "lmstudio"); err != nil { + if err := ex.Install(context.Background(), "lmstudio", true); err != nil { t.Fatal(err) } - if err := ex.Uninstall(context.Background(), "lmstudio"); err != nil { + if err := ex.Uninstall(context.Background(), "lmstudio", true); err != nil { t.Fatal(err) } } @@ -98,7 +98,7 @@ func TestDetectedExternalEngineDoesNotAcquirePathOwnership(t *testing.T) { // while silently leaving PATH alone. func TestManagedInstallWithNoReceiptStillClaimsPath(t *testing.T) { ex, cli, _ := pathLifecycleExecutor(t, "ollama", "process") - if err := ex.Install(context.Background(), "ollama"); err != nil { + if err := ex.Install(context.Background(), "ollama", true); err != nil { t.Fatal(err) } if err := os.Remove(ex.pathReceiptFile("ollama")); err != nil { @@ -107,7 +107,7 @@ func TestManagedInstallWithNoReceiptStillClaimsPath(t *testing.T) { 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"); err != nil { + if err := ex.Install(context.Background(), "ollama", true); err != nil { t.Fatal(err) } if added != filepath.Dir(cli) { @@ -120,7 +120,7 @@ func TestManagedInstallWithNoReceiptStillClaimsPath(t *testing.T) { // 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"); err != nil { + if err := ex.Install(context.Background(), "ollama", true); err != nil { t.Fatal(err) } before, err := os.ReadFile(profile) @@ -142,7 +142,7 @@ func TestMovedCLIMigratesTheOwnedPathEntry(t *testing.T) { st.plat.Runtime.CLI = moved // Already detected, so this is the short-circuit path. - if err := ex.Install(context.Background(), "ollama"); err != nil { + if err := ex.Install(context.Background(), "ollama", true); err != nil { t.Fatal(err) } after, err := os.ReadFile(profile) @@ -164,7 +164,7 @@ func TestMovedCLIMigratesTheOwnedPathEntry(t *testing.T) { // 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"); err != nil { + if err := ex.Install(context.Background(), "ollama", true); err != nil { t.Fatal(err) } orphaned, err := os.ReadFile(profile) @@ -179,7 +179,7 @@ func TestReinstallReadoptsItsOwnOrphanedProfileBlock(t *testing.T) { if err := os.RemoveAll(ex.pathReceiptDir()); err != nil { t.Fatal(err) } - if err := ex.Install(context.Background(), "ollama"); err != nil { + if err := ex.Install(context.Background(), "ollama", true); err != nil { t.Fatal(err) } after, err := os.ReadFile(profile) @@ -189,7 +189,7 @@ func TestReinstallReadoptsItsOwnOrphanedProfileBlock(t *testing.T) { if string(after) != string(orphaned) { t.Fatalf("reinstall duplicated the block: %q", after) } - if err := ex.Uninstall(context.Background(), "ollama"); err != nil { + if err := ex.Uninstall(context.Background(), "ollama", true); err != nil { t.Fatal(err) } remaining, err := os.ReadFile(profile) @@ -205,7 +205,7 @@ func TestReinstallReadoptsItsOwnOrphanedProfileBlock(t *testing.T) { // receipts live in. func TestDrainReleasesThePathBlock(t *testing.T) { ex, _, profile := pathLifecycleExecutor(t, "ollama", "process") - if err := ex.Install(context.Background(), "ollama"); err != nil { + if err := ex.Install(context.Background(), "ollama", true); err != nil { t.Fatal(err) } contents, err := os.ReadFile(profile) @@ -233,7 +233,7 @@ func TestDrainReleasesThePathBlock(t *testing.T) { // 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"); err != nil { + if err := ex.Install(context.Background(), "ollama", true); err != nil { t.Fatal(err) } if err := drainUserPaths(ex.baseDir, removeShellPath); err != nil { @@ -278,7 +278,7 @@ func TestDrainRemovesARecordWithNoInstallBehindIt(t *testing.T) { 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"); err != nil { + if err := ex.Install(context.Background(), "lmstudio", true); err != nil { t.Fatal(err) } // The application uninstaller: release the entries, leave the engine. @@ -287,7 +287,7 @@ func TestReinstallRepublishesPathForAnEngineOutsideTheInstallDirectory(t *testin } var added string ex.addToPath = func(dir string, _ *pathReceipt, _ func() error) error { added = dir; return nil } - if err := ex.Install(context.Background(), "lmstudio"); err != nil { + if err := ex.Install(context.Background(), "lmstudio", true); err != nil { t.Fatal(err) } if added != filepath.Dir(cli) { @@ -308,7 +308,7 @@ func TestSkippingAnExternalInstallLeavesTheWarningStanding(t *testing.T) { } } ex.reporter.report(serviceError{ID: pathFailedID("lmstudio"), Message: "an earlier attempt failed", Severity: "warning"}) - if err := ex.Install(context.Background(), "lmstudio"); err != nil { + if err := ex.Install(context.Background(), "lmstudio", true); err != nil { t.Fatal(err) } if pathWarning(ex, "lmstudio") == "" { @@ -320,14 +320,14 @@ func TestSkippingAnExternalInstallLeavesTheWarningStanding(t *testing.T) { // 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"); err != nil { + 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") + err := ex.Uninstall(context.Background(), "ollama", true) if !errors.Is(err, errUnreadableReceipt) { t.Fatalf("uninstall returned %v, want the unreadable record to surface", err) } @@ -417,7 +417,7 @@ func TestPathLockWaitIsBounded(t *testing.T) { // 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"); err != nil { + 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 { @@ -425,20 +425,20 @@ func TestUnreadableReceiptIsTreatedAsNoClaim(t *testing.T) { } var added string ex.addToPath = func(dir string, _ *pathReceipt, _ func() error) error { added = dir; return nil } - if err := ex.Install(context.Background(), "ollama"); err != 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"); err != nil { + 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"); err != nil { + if err := ex.Install(context.Background(), "ollama", true); err != nil { t.Fatal(err) } before, err := os.ReadFile(profile) @@ -452,7 +452,7 @@ func TestFailedUninstallPreservesPathOwnership(t *testing.T) { // 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"); err == nil { + if err := ex.Uninstall(context.Background(), "ollama", true); err == nil { t.Fatal("expected failed uninstall") } after, err := os.ReadFile(profile) @@ -469,19 +469,19 @@ func TestFailedUninstallPreservesPathOwnership(t *testing.T) { func TestPathCleanupCanRetryAfterEngineRemoval(t *testing.T) { ex, cli, profile := pathLifecycleExecutor(t, "lmstudio", "command") - if err := ex.Install(context.Background(), "lmstudio"); err != nil { + 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"); !errors.Is(err, 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"); err != nil { + if err := ex.Uninstall(context.Background(), "lmstudio", true); err != nil { t.Fatal(err) } contents, err := os.ReadFile(profile) 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/spec.md b/services/nvpair-engine-manager/spec.md index 8afe3e92..61c4f653 100644 --- a/services/nvpair-engine-manager/spec.md +++ b/services/nvpair-engine-manager/spec.md @@ -33,12 +33,13 @@ 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, then publishes the engine's CLI directory on this user's PATH. +- **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; a declined uninstall leaves the entries and deletes the record, handing them to the user. `EngineStatus.path_managed` tells a client whether there is an entry to ask about. - **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 the download and the install command. 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 — 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. @@ -80,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 } ``` @@ -115,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 and PATH publication) | -| `engine:uninstall` | `{ engine }` | `EngineStatus` (after removal and PATH cleanup) | +| `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` | @@ -169,7 +171,7 @@ The `engine:remote-*` methods are the client half: engine-manager resolves the t ## 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 the download, and the PATH step re-runs but changes nothing it already owns. +- **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. 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-tui/ui/engines.go b/services/nvpair-tui/ui/engines.go index 809febfb..4001b5e2 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, true) + case "n", "N": + v.confirm = nil + return v.runLifecycle(p.method, p.what, 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,41 @@ 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, false), true +} + +// 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 treats a +// missing or false path as no consent. +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, path bool) 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, lifecycleParams(engine, path), func(_ *rpc.Message, err error) tea.Msg { return engineOpMsg{what: what, engine: engine, err: err} - }), true + }) } func (v *enginesView) selectedEngine() string { @@ -326,6 +379,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..ff94cfeb 100644 --- a/services/nvpair-tui/ui/engines_test.go +++ b/services/nvpair-tui/ui/engines_test.go @@ -3,7 +3,69 @@ 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) + } + } +} // TestPullParamsSendsBothKeys guards the LM Studio pull fix: the pull params // must carry the model under BOTH "name" (Ollama's /api/pull body key) and From 535371959ee6f00d50fc3c99421f4e5104668528 Mon Sep 17 00:00:00 2001 From: Chris Kelsey Date: Mon, 28 Sep 2026 15:44:02 -0700 Subject: [PATCH 3/4] fix: keep PATH claims on unasked uninstalls and keep path_managed current Addresses review of the PATH consent change: - engine:uninstall without path keeps PAIR's claim instead of handing the entries to the user, so a stale client can never strand them. - engine:state-changed is emitted after every PATH step, and desktop update reads path_managed fresh from engine:status. - path_managed counts only entries PAIR wrote; peers never see it. - A consented uninstall always deletes the record; peer installs record the installed flag so a later local install can adopt them. - Debian releases PATH entries only on purge, from a copy of the engine manager kept by prerm, and keeps data when the release fails. - Windows retries a record rename that races an unlocked status read. - Error-banner uninstall retry no longer defaults to removing the entry. Signed-off-by: Chris Kelsey --- desktop/docs/frontend-api.md | 11 +- desktop/electron-builder.config.ts | 6 +- desktop/scripts/build/linux/after-remove.sh | 23 ++- desktop/scripts/build/linux/before-remove.sh | 56 +++---- .../electron/service-bridge/empty-handlers.ts | 69 +++++--- desktop/src/shared/types/engine-api.ts | 5 +- desktop/src/ui/api/engine-api.ts | 8 +- .../ui/components/BackendRow/BackendRow.tsx | 2 +- desktop/src/ui/components/ErrorModal.tsx | 7 +- .../tests/modular/engine-path-consent.test.ts | 46 +++++- docs/engine-lifecycle.mdx | 4 +- services/nvpair-engine-manager/README.md | 29 ++-- .../nvpair-engine-manager/controlserver.go | 8 +- .../nvpair-engine-manager/controlstream.go | 1 + services/nvpair-engine-manager/executor.go | 3 +- services/nvpair-engine-manager/install.go | 65 ++++++-- .../install_path_test.go | 150 +++++++++++++++++- services/nvpair-engine-manager/manager.go | 17 +- .../nvpair-engine-manager/pathownership.go | 60 ++++++- services/nvpair-engine-manager/setport.go | 2 +- services/nvpair-engine-manager/spec.md | 6 +- .../nvpair-engine-manager/syncdir_unix.go | 4 + .../nvpair-engine-manager/syncdir_windows.go | 32 ++++ .../syncdir_windows_test.go | 46 ++++++ services/nvpair-tui/ui/engines.go | 21 ++- services/nvpair-tui/ui/engines_test.go | 3 + 26 files changed, 547 insertions(+), 137 deletions(-) create mode 100644 services/nvpair-engine-manager/syncdir_windows_test.go diff --git a/desktop/docs/frontend-api.md b/desktop/docs/frontend-api.md index 86ed8374..caacf570 100644 --- a/desktop/docs/frontend-api.md +++ b/desktop/docs/frontend-api.md @@ -118,10 +118,13 @@ environment assignments pass through without an engine-option catalog. - `setModelExpiry`. `install` and `uninstall` carry `path`, the user's answer to changing their -PATH. Absent means no consent. 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`. Remote installs never change -PATH, and `update` reuses the existing `pathManaged` answer without asking. +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 diff --git a/desktop/electron-builder.config.ts b/desktop/electron-builder.config.ts index 612bb7be..2407e9e8 100644 --- a/desktop/electron-builder.config.ts +++ b/desktop/electron-builder.config.ts @@ -393,9 +393,9 @@ const config: Configuration = { afterInstall: 'scripts/build/linux/after-install.sh', afterRemove: 'scripts/build/linux/after-remove.sh', // prerm has no dedicated option, so it goes through the raw fpm passthrough. - // It has to be prerm rather than postrm because dpkg deletes the package's - // files before postrm runs, and the PATH cleanup runs a binary from /opt — - // see before-remove.sh. Unlike afterInstall/afterRemove, fpm arguments are + // 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. 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 index cfbfaa1c..6daa4efd 100644 --- a/desktop/scripts/build/linux/before-remove.sh +++ b/desktop/scripts/build/linux/before-remove.sh @@ -2,32 +2,29 @@ # SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. # SPDX-License-Identifier: Apache-2.0 -# Personal AI Router Debian pre-remove. Releases the PATH entries this user's -# engines own. No `set -e`, and a trailing `exit 0`, so a best-effort step never -# fails the removal. +# 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 prerm AND NOT postrm. 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. -# after-remove handles the data root on `purge` -- but dpkg deletes the -# package's files before postrm runs, so by then the binary that understands -# those records is gone. prerm is the last point at which both still exist. +# 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. # -# This runs on `apt remove` as well as `apt purge`, so a plain remove gives up -# its PATH entries even though it keeps the engines. That is the recoverable -# direction: the drain releases each entry but keeps the record's note that PAIR -# installed the engine, so a reinstall re-adopts it and republishes. Deleting -# the record outright is what made this unrecoverable for an engine whose CLI -# lives outside PAIR's install directory -- nothing else tells it apart from an -# engine the user installed. The alternative -- entries no tool can identify, -# pointing into a directory a later purge deletes -- is worse either way. +# 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 leave PATH alone. +# recovering from one. Those keep the installation, so there is nothing to keep. case "${1:-}" in upgrade|failed-upgrade) exit 0 @@ -36,26 +33,15 @@ 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 "${DPKG_MAINTSCRIPT_PACKAGE:-}" 2>/dev/null \ +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 -# prerm runs as root, while the records, the dotfiles, and $XDG_CONFIG_HOME all -# belong to the user who ran the app. Resolve that user the same way -# after-remove resolves the data root on purge, and run as them so the binary -# reads the environment it wrote under. Best-effort: on a multi-user box, other -# users' entries are left for their own reinstall to reclaim. -real_user="${SUDO_USER:-}" -if [ -z "$real_user" ] && command -v logname >/dev/null 2>&1; then - real_user="$(logname 2>/dev/null || true)" -fi -[ -n "$real_user" ] && [ "$real_user" != root ] || exit 0 - -if command -v runuser >/dev/null 2>&1; then - runuser -u "$real_user" -- "$engine_manager" --remove-user-path >/dev/null 2>&1 || true -else - su -s /bin/sh -c "'$engine_manager' --remove-user-path" "$real_user" >/dev/null 2>&1 || true -fi +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/src/electron/service-bridge/empty-handlers.ts b/desktop/src/electron/service-bridge/empty-handlers.ts index 730958b7..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 @@ -210,39 +250,20 @@ function routeEngineManagerCommand(payload: WsInvokeRequest<'engine:command'>): 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, path: payload.path === true }, + 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`. - // An update carries the user's earlier PATH answer through both - // steps rather than asking again. - const path = state.localEnginePathManaged(payload.engineType) + case 'update': state.beginLocalEngineOp(payload.engineType, 'installing') - supervisor.sendProcess( - 'broker', - 'engine:uninstall', - { engine, path }, - failPendingOp('update', 'uninstall'), - true - ) - supervisor.sendProcess( - 'broker', - 'engine:install', - { engine, start: true, path }, - failPendingOp('update', 'install'), - true - ) + void updateLocalEngine(engine, payload.engineType, failPendingOp) break - } case 'toggle': void toggleLocalEngine(engine, payload.engineType) break diff --git a/desktop/src/shared/types/engine-api.ts b/desktop/src/shared/types/engine-api.ts index 68bffa80..d411200d 100644 --- a/desktop/src/shared/types/engine-api.ts +++ b/desktop/src/shared/types/engine-api.ts @@ -59,8 +59,9 @@ export interface EngineCommandPayload { 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). Absent means - * no consent, so PATH is left as it is. + * 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 } diff --git a/desktop/src/ui/api/engine-api.ts b/desktop/src/ui/api/engine-api.ts index 95f78518..af6af7a3 100644 --- a/desktop/src/ui/api/engine-api.ts +++ b/desktop/src/ui/api/engine-api.ts @@ -51,10 +51,12 @@ export interface IEngineApi { /** 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, - * removing the PATH entry PAIR added when the user agreed. + * 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 + 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. */ diff --git a/desktop/src/ui/components/BackendRow/BackendRow.tsx b/desktop/src/ui/components/BackendRow/BackendRow.tsx index c6cd1dc6..efeba440 100644 --- a/desktop/src/ui/components/BackendRow/BackendRow.tsx +++ b/desktop/src/ui/components/BackendRow/BackendRow.tsx @@ -172,7 +172,7 @@ export function BackendRow({ ) const handleUninstall = useCallback(() => { - window.pairApi.engines.uninstall(backend.type, nodeId, pathManaged && removePath) + window.pairApi.engines.uninstall(backend.type, nodeId, pathManaged ? removePath : undefined) }, [backend.type, nodeId, pathManaged, removePath]) const handleUpdate = useCallback(() => { diff --git a/desktop/src/ui/components/ErrorModal.tsx b/desktop/src/ui/components/ErrorModal.tsx index 04e6ca1d..4d7f4d9b 100644 --- a/desktop/src/ui/components/ErrorModal.tsx +++ b/desktop/src/ui/components/ErrorModal.tsx @@ -188,7 +188,7 @@ function ErrorBanner({ ) const [askInstallPath, setAskInstallPath] = useState(false) const [askUninstallPath, setAskUninstallPath] = useState(false) - const [removePath, setRemovePath] = useState(true) + const [removePath, setRemovePath] = useState(false) // Clearing the error unmounts this banner, so it waits for any PATH answer. const rerun = (run: () => void) => { @@ -210,10 +210,11 @@ function ErrorBanner({ return } if (pathManaged) { - setRemovePath(true) + // Unchecked: the failed attempt may have been one that kept the entry. + setRemovePath(false) setAskUninstallPath(true) } else { - rerun(() => window.pairApi.engines.uninstall(engineType, nodeId, false)) + rerun(() => window.pairApi.engines.uninstall(engineType, nodeId)) } } diff --git a/desktop/tests/modular/engine-path-consent.test.ts b/desktop/tests/modular/engine-path-consent.test.ts index 6297eb02..aefddb8d 100644 --- a/desktop/tests/modular/engine-path-consent.test.ts +++ b/desktop/tests/modular/engine-path-consent.test.ts @@ -64,18 +64,46 @@ describe('engine PATH consent on local lifecycle commands', () => { ]) }) - it('treats an omitted answer as no consent', async () => { + 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: 'uninstall', + command: 'update', engineType: 'ollama', nodeId: 'local-node' }) - expect(sentParams('engine:uninstall')).toEqual([{ engine: 'ollama', path: false }]) + await vi.waitFor(() => { + expect(sentParams('engine:uninstall')).toEqual([{ engine: 'ollama', path: true }]) + expect(sentParams('engine:install')).toEqual([ + { engine: 'ollama', start: true, path: true } + ]) + }) }) - it('carries the existing PATH choice through both steps of an update', async () => { + 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', @@ -83,9 +111,11 @@ describe('engine PATH consent on local lifecycle commands', () => { nodeId: 'local-node' }) - expect(sentParams('engine:uninstall')).toEqual([{ engine: 'ollama', path: true }]) - expect(sentParams('engine:install')).toEqual([ - { engine: 'ollama', start: true, path: true } - ]) + await vi.waitFor(() => { + expect(sentParams('engine:uninstall')).toEqual([{ engine: 'ollama' }]) + expect(sentParams('engine:install')).toEqual([ + { engine: 'ollama', start: true, path: false } + ]) + }) }) }) diff --git a/docs/engine-lifecycle.mdx b/docs/engine-lifecycle.mdx index 7a24a819..a9c2ad46 100644 --- a/docs/engine-lifecycle.mdx +++ b/docs/engine-lifecycle.mdx @@ -90,7 +90,9 @@ If PAIR added an engine to your PATH, the uninstall confirmation includes 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. +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: diff --git a/services/nvpair-engine-manager/README.md b/services/nvpair-engine-manager/README.md index ceaf8900..0ef07e49 100644 --- a/services/nvpair-engine-manager/README.md +++ b/services/nvpair-engine-manager/README.md @@ -52,8 +52,10 @@ Requests (caller → service): | `log/set-level` | `{ level }` | `{ level }` | `EngineStatus` = `{ engine, display_name, installed, running, healthy, port, path_managed }`. -`path_managed` is true while PAIR owns a PATH entry for the engine on this -machine, so a client asks about removing one only when there is one. +`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 @@ -63,13 +65,14 @@ 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. Windows updates `HKCU\Environment\Path`; Unix appends a block to the login +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 — skips this -step entirely. A PATH failure is a dismissible warning, never an install error, +`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 @@ -81,15 +84,19 @@ 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; 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 without -`path:true` leaves the entries in place and deletes the receipt, so they become -the user's and `--remove-user-path` no longer touches them. +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. +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. 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 f4b554dd..86104ff9 100644 --- a/services/nvpair-engine-manager/controlstream.go +++ b/services/nvpair-engine-manager/controlstream.go @@ -92,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/executor.go b/services/nvpair-engine-manager/executor.go index 0e63907d..08687305 100644 --- a/services/nvpair-engine-manager/executor.go +++ b/services/nvpair-engine-manager/executor.go @@ -36,7 +36,8 @@ type EngineStatus struct { 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. + // 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"` } diff --git a/services/nvpair-engine-manager/install.go b/services/nvpair-engine-manager/install.go index d22d0823..7f68aad2 100644 --- a/services/nvpair-engine-manager/install.go +++ b/services/nvpair-engine-manager/install.go @@ -25,16 +25,30 @@ import ( type installPathMode int const ( - // pathUntouched leaves PATH and the ownership record alone. - pathUntouched installPathMode = iota // 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 + 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. @@ -53,8 +67,12 @@ func (e *Executor) Install(ctx context.Context, engine string, addToPath bool) e // 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, pathUntouched) + return e.install(ctx, engine, pathRecordOnly) } func (e *Executor) install(ctx context.Context, engine string, pathMode installPathMode) error { @@ -72,6 +90,11 @@ func (e *Executor) install(ctx context.Context, engine string, pathMode installP } 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() @@ -259,6 +282,20 @@ func managedCLI(st *engineState) bool { // 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 @@ -269,7 +306,9 @@ func (e *Executor) Uninstall(ctx context.Context, engine string, removePath bool // 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. - return errors.Join(e.setDesiredEnabled(engine, false), e.finishUninstallPath(engine, removePath)) + 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 { @@ -338,18 +377,24 @@ func (e *Executor) Uninstall(ctx context.Context, engine string, removePath bool st.mu.Lock() st.binPath = "" st.mu.Unlock() - e.emitState(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. - return errors.Join(e.setDesiredEnabled(engine, false), e.finishUninstallPath(engine, removePath)) + 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 err } -func (e *Executor) finishUninstallPath(engine string, removePath bool) error { - if removePath { +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) } - return e.releasePathOwnership(engine) } // maxDownloadBytes caps a single engine download (engine installers / diff --git a/services/nvpair-engine-manager/install_path_test.go b/services/nvpair-engine-manager/install_path_test.go index 168e19ea..3d46de58 100644 --- a/services/nvpair-engine-manager/install_path_test.go +++ b/services/nvpair-engine-manager/install_path_test.go @@ -14,6 +14,7 @@ import ( "path/filepath" "runtime" "strings" + "sync" "testing" ) @@ -312,7 +313,11 @@ 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, _ *pathReceipt, _ func() error) error { added = dir; return nil } + 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") @@ -412,9 +417,9 @@ func TestUninstallRPCRemovesPathOnlyWithConsent(t *testing.T) { t.Error("status still claims the removed PATH entry") } }) - t.Run("omitted path keeps the entry and gives up the claim", func(t *testing.T) { + 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"}`) + 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") } @@ -425,6 +430,110 @@ func TestUninstallRPCRemovesPathOnlyWithConsent(t *testing.T) { 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 @@ -459,7 +568,38 @@ func TestRemoteInstallDoesNotChangeTheTargetUsersPath(t *testing.T) { if !fileExists(cli) { t.Fatal("remote install did not install the engine") } - if _, err := os.Stat(ex.pathReceiptFile("lmstudio")); !errors.Is(err, os.ErrNotExist) { - t.Fatalf("remote install claimed PATH ownership: %v", err) + 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/manager.go b/services/nvpair-engine-manager/manager.go index a22969f3..1c68aced 100644 --- a/services/nvpair-engine-manager/manager.go +++ b/services/nvpair-engine-manager/manager.go @@ -36,15 +36,16 @@ 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 consent to change their PATH: on install, to publish the -// engine's CLI directory; on uninstall, to remove what PAIR published. It is -// absent unless a client asked the user, so an omitted field never touches it. +// 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"` + Path *bool `json:"path,omitempty"` } type actionParam struct { @@ -312,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, p.Path); 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, p.Path) + 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/pathownership.go b/services/nvpair-engine-manager/pathownership.go index d8b67fcc..b6f7d401 100644 --- a/services/nvpair-engine-manager/pathownership.go +++ b/services/nvpair-engine-manager/pathownership.go @@ -26,8 +26,7 @@ type pathReceipt struct { // 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, by - // deleting the whole record. + // 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 @@ -35,6 +34,13 @@ type pathReceipt struct { 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"` @@ -234,8 +240,13 @@ func (e *Executor) uninstallPath(engine string) error { receipt, err := loadPathReceipt(file) if err == nil && receipt.Dir != "" { err = e.removeFromPath(receipt) - if err == nil { - err = os.Remove(file) + } + // 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 { @@ -254,6 +265,10 @@ func (e *Executor) uninstallPath(engine string) error { // 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 { @@ -291,16 +306,47 @@ func (e *Executor) releasePathOwnership(engine string) error { 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. Records are published by -// atomic rename, so the read needs no lock. +// 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.Dir != "" + return err == nil && receipt.ownsEntries() } // removeAllUserPaths releases every PATH entry the engines under baseDir own. diff --git a/services/nvpair-engine-manager/setport.go b/services/nvpair-engine-manager/setport.go index 8cfeb945..9f342e5c 100644 --- a/services/nvpair-engine-manager/setport.go +++ b/services/nvpair-engine-manager/setport.go @@ -246,7 +246,7 @@ func writeJSONAtomic(path string, v any) error { if err := writeAndSync(file, data, 0o600); err != nil { return fmt.Errorf("write %s: %w", name, err) } - if err := os.Rename(tmp, path); err != nil { + 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 61c4f653..25154b74 100644 --- a/services/nvpair-engine-manager/spec.md +++ b/services/nvpair-engine-manager/spec.md @@ -34,7 +34,7 @@ A declarative, config-driven control plane for **local inference engines** (Olla ## 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, 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; a declined uninstall leaves the entries and deletes the record, handing them to the user. `EngineStatus.path_managed` tells a client whether there is an entry to ask about. +- **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.) @@ -166,7 +166,7 @@ The `engine:remote-*` methods are the client half: engine-manager resolves the t - **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 deletes the record. The application uninstaller (`--remove-user-path`) 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. +- **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). @@ -185,7 +185,7 @@ The `engine:remote-*` methods are the client half: engine-manager resolves the t - **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 and treated as no claim; a managed install with no receipt reacquires PATH rather than reporting a silent success. +- **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/syncdir_unix.go b/services/nvpair-engine-manager/syncdir_unix.go index 74c257c2..f745320e 100644 --- a/services/nvpair-engine-manager/syncdir_unix.go +++ b/services/nvpair-engine-manager/syncdir_unix.go @@ -35,3 +35,7 @@ func syncDir(dir string) error { } 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 index 906c86b0..f2145c87 100644 --- a/services/nvpair-engine-manager/syncdir_windows.go +++ b/services/nvpair-engine-manager/syncdir_windows.go @@ -3,6 +3,38 @@ 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-tui/ui/engines.go b/services/nvpair-tui/ui/engines.go index 4001b5e2..53a2e5a7 100644 --- a/services/nvpair-tui/ui/engines.go +++ b/services/nvpair-tui/ui/engines.go @@ -196,10 +196,10 @@ func (v *enginesView) handleKey(msg tea.KeyMsg) tea.Cmd { switch msg.String() { case "y", "Y": v.confirm = nil - return v.runLifecycle(p.method, p.what, p.engine, true) + 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, false) + 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" @@ -296,7 +296,13 @@ func (v *enginesView) handleAction(msg tea.KeyMsg) (tea.Cmd, bool) { v.confirm = &pathPrompt{method: method, what: what, engine: engine, question: question} return nil, true } - return v.runLifecycle(method, what, engine, false), 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 @@ -316,15 +322,16 @@ func pathQuestion(method string, e engineStatus) string { return "" } -// lifecycleParams carries the user's PATH answer; the engine-manager treats a -// missing or false path as no consent. +// 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, path bool) tea.Cmd { +func (v *enginesView) runLifecycle(method, what, engine string, params map[string]any) tea.Cmd { v.status = what + " " + engine + "..." - return call(v.client, method, lifecycleParams(engine, path), 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} }) } diff --git a/services/nvpair-tui/ui/engines_test.go b/services/nvpair-tui/ui/engines_test.go index ff94cfeb..0ab58e20 100644 --- a/services/nvpair-tui/ui/engines_test.go +++ b/services/nvpair-tui/ui/engines_test.go @@ -65,6 +65,9 @@ func TestLifecycleParamsCarryThePathAnswer(t *testing.T) { 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 From cb132b941fa8199f8b4af576a72c34030e9ac780 Mon Sep 17 00:00:00 2001 From: Chris Kelsey Date: Mon, 28 Sep 2026 15:58:20 -0700 Subject: [PATCH 4/4] fix(desktop): shorten the Add to PATH question Signed-off-by: Chris Kelsey --- desktop/src/ui/components/EnginePathConsentModal.tsx | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/desktop/src/ui/components/EnginePathConsentModal.tsx b/desktop/src/ui/components/EnginePathConsentModal.tsx index 1b42ddaf..84910569 100644 --- a/desktop/src/ui/components/EnginePathConsentModal.tsx +++ b/desktop/src/ui/components/EnginePathConsentModal.tsx @@ -33,10 +33,6 @@ export function EnginePathConsentModal({ onCancel: () => void }) { const names = engineList.format(engines.map(engine => EngineDisplayNames[engine])) - const edited = - window.windowApi.platform === 'Windows' - ? 'your user environment variables' - : 'your shell profile' return (
Add the {names} command-line tools to your PATH so you can run them - from a terminal? PAIR will update {edited}. Open a new terminal - afterward for the change to take effect. + from a terminal?