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, " \""), "/", `\`), `\`) +}