Repository navigation
Fix/review follow ups - #153
Merged
Merged
Conversation
Noah-Tervalon-Nvidia
added this pull request to stack #149
October 8, 2026 12:47
kjlubick
self-requested a review
October 8, 2026 15:56
kjlubick
approved these changes
Oct 8, 2026
kjlubick
left a comment
kjlubick
left a comment
Collaborator
There was a problem hiding this comment.
LGTM with small changes
Noah-Tervalon-Nvidia
force-pushed
the
fix/review-follow-ups
branch
2 times, most recently
from
October 8, 2026 17:03
bb0596a to
beb4b7c
Compare
removeTreePreserving unlinked a symlink rather than descending into it, so an uninstall could not be steered into another tree. It tested for ModeSymlink. Since Go 1.23 a Windows junction is reported as an irregular file instead -- neither a symlink nor a directory -- so a junctioned remove target, or a junction on the way to the model store, was read with ReadDir and the directory it names emptied. The Windows uninstaller runs this elevated, and both a junction and the install marker can be made without privilege, so a standard user could aim an administrator's "remove all data" at a protected directory. Descend only into what Lstat reports as a real directory and unlink everything else. The test that called itself the junction case created a symlink, which Go reports differently; two Windows tests now use real junctions. CI runs Go tests on Linux only, so they run on Windows machines rather than in CI. Signed-off-by: Terve <ntervalon@nvidia.com>
The model store was matched by comparing path strings, folded for case on Windows and macOS. Two consequences: - A store that is a symlink lost its models. With ~/.lmstudio/models linking to ~/.lmstudio/weights, removing ~/.lmstudio kept the link and deleted weights as an unrelated sibling. - Case handling followed the operating system rather than the filesystem. A macOS volume can be case-sensitive, and a Linux mount can ignore case, so folding by GOOS could both keep and delete the wrong directory. Compare files instead (os.SameFile): the store is the path as named and the directory it resolves to, and the directories above it are taken along both the written and the resolved path. The filesystem then says what is the store, whatever the spelling. pathWithinRoot goes back to the exact comparison it had before case folding was added; its remaining callers compare paths they built themselves. Manifest validation now compares without case on every platform, since one manifest runs on filesystems of both kinds. Tests cover a symlinked store, a store two levels down (the first test to exercise the recursion), and a store spelled in another case, which runs only where the filesystem ignores case. Signed-off-by: Terve <ntervalon@nvidia.com>
"Reset all data and quit" asked the engine manager to remove the engines PAIR installed, then wiped the data directory whatever the answer. The wipe deletes the install markers -- the only record that an engine is PAIR's -- so an engine that could not be removed, LM Studio in the user's home most of all, was left on disk with nothing allowed to remove it afterwards. Wipe only once every engine PAIR installed is gone. If one could not be removed, or the engine manager could not be asked, the reset stops before deleting anything, names the engine on the status line, and gives the keyboard back so the operator can try again or uninstall it first. An engine PAIR did not install is still skipped, not failed. Signed-off-by: Terve <ntervalon@nvidia.com>
…ll on disk Settings > Reset app data treated engine removal as best-effort: a failed removal, a failed call, or a service that was not running were logged, and the app data folder was deleted anyway. That folder holds the install markers, the only record that an engine is PAIR's, so the engine that could not be removed was left behind for good. Stop before the teardown instead, with an error the Settings card already shows: which engine could not be removed, or that the service is not running. The confirmation now says a reset can stop. The orchestrator had no tests. These cover the wipe going ahead once every engine PAIR installed is gone, and stopping, with nothing torn down, for each of the three ways removal can fail. Signed-off-by: Terve <ntervalon@nvidia.com>
The uninstall changes added a progress line to the Windows uninstaller, an engine-manager error a user reads when an uninstall is declined, and comments in the uninstallers, all naming the product "Personal AI Router". The app is NVIDIA PAIR everywhere a user sees it. The data folder and the Windows firewall rule names keep "Personal AI Router": those are an on-disk path and stable identifiers, not branding. Signed-off-by: Terve <ntervalon@nvidia.com>
Review asked for the expected error to be stated rather than searched for. The over-long query's error is a fixed sentence, so the test now compares it whole. The failed search's error ends in the operating system's connection error, which names a random port, so it cannot be; the test now checks the one thing that has to hold, that no *url.Error -- the error that carries the search URL -- survives in the chain. Signed-off-by: Terve <ntervalon@nvidia.com>
…dels folder Getting started now says what to do when Restart & install finishes the update from PAIR.app to NVIDIA PAIR.app without reopening the app, and names the app NVIDIA PAIR in the macOS install step, which still said PAIR after the rename. Engine lifecycle now states the one case where removing LM Studio deletes models: a models folder the user moved elsewhere inside ~/.lmstudio. PAIR preserves the engine's default store, and since the store is now recognised by identity, one linked back to ~/.lmstudio/models is kept. Signed-off-by: Terve <ntervalon@nvidia.com>
The spec said model stores live outside the data folder, but not how an engine's removal treats them. State it: only the paths uninstall.remove names are deleted, the store is matched by file identity rather than path, links and junctions are unlinked rather than followed, and a target that is the store or inside it is refused. Signed-off-by: Terve <ntervalon@nvidia.com>
…er chose --uninstall-managed builds its registry from the bundled manifests only, so a user-writable override cannot choose what an elevated uninstaller runs or deletes. That also dropped the user's port. Before removing an engine, Uninstall checks its port to stop the instance it owns or to refuse a foreign service there; with the default port in hand it looked in the wrong place for an engine the user had moved, and could refuse outright if something else answered on the default. Take the port, and only the port, from each override. A port decides where the uninstaller looks; what it may stop is decided by the managed binary's path, which stays bundled, and so do the commands and the paths it deletes. Signed-off-by: Terve <ntervalon@nvidia.com>
…deleted The only failure case for UninstallManaged declared no removal targets, so it never reached a removal that deletes some files and fails on others -- the case the loop's continue exists for. The new test makes one engine's binary undeletable: that engine reports a failure, keeps its install marker, loses what could be deleted and keeps its models, and the engine after it is still removed. Signed-off-by: Terve <ntervalon@nvidia.com>
…able Review asked whether curl's exit codes differ across platforms. They do not: 22 is curl's own code for an HTTP error under --fail. On Windows exec.Command runs curl.exe, which ships with the OS, not PowerShell's curl alias, and a host with no curl at all gets a start error rather than an exit code, which the test already skips. Signed-off-by: Terve <ntervalon@nvidia.com>
The README and the engine lifecycle page said removing PAIR, or resetting its data, removes the engines PAIR installed. A reset now stops and says which engine it could not remove, but a platform uninstaller cannot stop halfway: it leaves that engine in place and finishes. Say so, and where to remove it from. Signed-off-by: Terve <ntervalon@nvidia.com>
…age does The _windows suffix already limits the file to Windows builds. Every other OS-specific test in the package also states it with a //go:build line, so this one does too. Signed-off-by: Terve <ntervalon@nvidia.com>
Only a real directory can lead to the model store, so under now returns false for anything else rather than relying on its caller to check first. Signed-off-by: Terve <ntervalon@nvidia.com>
…arately One condition checked both that there was an error and what it said. Split, the test fails plainly if the query is answered, and compares the message only once there is one -- comparing a nil error's message would panic instead of failing. Signed-off-by: Terve <ntervalon@nvidia.com>
The uninstaller took only the port from a user's override. The running engine manager reads the whole override, so a reset kept a models_dir the user had pointed elsewhere while the uninstaller kept only the bundled default -- and removed the user's store if it sat inside a removal target. Take models_dir along with the port. Both say where the engine's things are rather than what to run or delete: a store decides only what removal keeps. The overlay is checked with the same validation startup applies, so a store that would hold a removal target, or a templated one, is ignored whole. Signed-off-by: Terve <ntervalon@nvidia.com>
Noah-Tervalon-Nvidia
force-pushed
the
fix/review-follow-ups
branch
from
October 8, 2026 17:19
beb4b7c to
2c87685
Compare
6 of 7 tasks
Noah-Tervalon-Nvidia
pushed a commit
that referenced
this pull request
Oct 9, 2026
…reenshots (#151) ## Description A documentation pass over the release that landed in `develop` with #147, #148, #117, #150, #145, and #153. It covers llama.cpp everywhere the docs list engines, replaces the terminal-interface screenshots of the old ten-tab interface, and corrects docs that no longer match the code. The branch was replayed onto `develop` after the stack was squash-merged, so it carries only the docs commits. <!-- pair-release-intent:v1 --> ### Changelog title n/a ### Changelog body n/a ### Bumps - services: none - nvpair-cluster-manager: none - nvpair-engine-manager: none - nvpair-errors: none - nvpair-job-scheduler: none - nvpair-manual-nodes: none - nvpair-node-info: none - nvpair-node-scanner: none - nvpair-node-settings: none - nvpair-proxy: none - nvpair-tui: none - nvpair-ui-broker: none - nvpair-workload-manager: none <!-- /pair-release-intent:v1 --> ## Scope **Terminal interface screenshots** (`docs/assets/onboarding/terminal-interface/`) - The three existing images showed the ten-tab interface from before #117. They are replaced with renders of the current Nodes, pairing, and Jobs views, at the same size as the originals. - New `04-tui-node-detail.png`: a machine's Engines and Models panes with Ollama, LM Studio, and llama.cpp. It is placed in "Look at One Machine". - The renders drive the real `ui` views and styles with sample data for two machines, so the layout, columns, and footer are the interface's own output. The harness that produced them is not part of this pull request. **Terminal interface capabilities** - `docs/known-issues.mdx` and `docs/engine-lifecycle.mdx` said the terminal interface cannot list or delete models, change ports, manage another node's engines, or show which node ran a job. It does all of these now. The lists keep only what is still true: updating an engine, sending your own inference request, installing PAIR updates, and surviving a dropped connection. - `docs/engine-lifecycle.mdx`: ports and startup arguments can be edited from a paired machine. Only uninstall, update, and CORS settings stay local. - `README.md`: leaving a cluster is `l` on the **Nodes** tab. There is no Cluster tab any more. - `docs/architecture.mdx`: the broker's `ping` is used by the **Service** tab, not a health view. **Terminal interface docs** (`docs/terminal-interface.mdx`, `services/nvpair-tui/README.md`), checked against the code: - It runs a broker and ten workers, not eleven. - While the interface runs, its own logs go to the Logs tab, not stderr. - The engine port, proxy port, and startup arguments are offered on a peer and written through `engine:apply-settings`. Only restart and uninstall are local-only. A peer's screen lists only its engine port, though `p` still changes that peer's proxy port. - Only a plain `go build` skips the update check. Builds made with `services/build.sh` or `services/build.bat` check. - `r` forgets a machine added with `f` without asking, and the "asks first" rule says so. The catalog filter also matches tags and clears with `c`. **llama.cpp coverage** - `README.md`: removed the claim that first-run setup does not preselect llama.cpp. `WELCOME_ENGINE_DEFAULT_SELECTED` preselects it. The macOS install step names **NVIDIA PAIR**. - `docs/getting-started.mdx`: - "Using an Engine's Own Command Line" gives llama.cpp's install path on Windows, Linux, and macOS, its cache, and querying its router on `8081`. The `OLLAMA_HOST` advice is marked as Ollama-only. - Ollama's path table gains a macOS row. - The endpoint table gains a llama.cpp row: OpenAI-compatible paths on `8080`, no Ollama `/api/...` and no Anthropic Messages API. - `docs/troubleshooting.mdx`: a `llama-server` you started yourself on `8080` hides requests from PAIR. The fix is to stop it and reopen PAIR, because the proxy keeps its fallback port until the broker restarts. - `docs/building.mdx` and `docs/engine-lifecycle.mdx`: model folders and example ports include `~/.llamacpp`, `~/.lmstudio/models`, and `8080`. - `services/installer/linux/INSTALL.md` and `services/installer/macos/INSTALL.md`: - Models live in each engine's own store, so deleting the bundle or configuration leaves them. - The bind-conflict note named `11435`, which is Ollama's relocated engine port, not a proxy port. It now lists the three proxy ports. - Linux: the bundle layout lists `nvpair-tui`, and the requirements say `x86_64` or `arm64`, which `installer_build.sh` produces. - Service and developer references that still assumed two engines: - the `nvpair-proxy` README and spec; - the `nvpair-job-scheduler` spec, including the `scheduler:get-status` shape; - the `nvpair-workload-manager` README and spec; - the `llamacpp-proxy` error IDs in `nvpair-errors`; - the `nvpair-ui-broker` README; - `.cursor/rules/proxy-inference-routing.mdc`; - `desktop/docs/services-backend.md`. - `services/nvpair-engine-manager/README.md` lists `engine:uninstall-managed`. `desktop/docs/services-parity.md` marks the pull `percent` as optional, after #145. **Not included** - Desktop screenshots that still show only Ollama, such as `getting-started/01-first-run-setup.png`. They need a capture from the Electron app. - `CHANGELOG.md`, which release-intent automation writes. ## Validation - Swept every `*.md`, `*.mdx`, and `.cursor/rules/*.mdc` file for engine lists that name Ollama or LM Studio without llama.cpp, and for references to the old terminal interface. Each hit was checked against the code. Statements that PAIR adopts an existing Ollama or LM Studio are left alone, because llama.cpp is never adopted. - Each factual change was checked against its source, including the engine manifests, `engines.All()`, `schedule.go`, `engineproxy.go`, `nodedetail.go`, `nodes.go`, `logoutput.go`, `updatecheck.go`, `services/build.sh`, `installer_build.sh`, and `appdir.go`. - A three-model review pass over the branch. Every finding was verified against the code before it was applied. - Reviewed each rendered screenshot against the doc text and alt text it accompanies. - `go vet ./ui/` in `services/nvpair-tui` after the harness was removed. - `python3 scripts/release-intent/validate_pr.py --description-file <this description> --skip-owned-files-check` ## Risk Documentation and images only. No code, build, or versioned file changes. Found while checking and left for a separate change: - In a colour terminal, an unplaced job's RAN ON cell renders as `c…` instead of `choosing`. The styled text is truncated by its byte width, and the tests render without colour, so they do not see it. - A comment in `ui/updatecheck.go` still says "eleven workers". - `nvpair-manual-nodes` probes hand-added nodes for Ollama and LM Studio only, not llama.cpp. - `installer_build.sh` does not copy `inference-dispatcher` into the services tarball, so the Jobs tab's test traffic cannot start from it. ## Checklist - [x] I have read the [Contributing Guidelines](https://github.com/NVIDIA/Personal-AI-Router/blob/main/CONTRIBUTING.md). - [x] Every commit is signed off (`git commit -s`), certifying the [Developer Certificate of Origin](https://developercertificate.org/). - [ ] New or existing tests cover the change. (Documentation only.) - [x] Relevant documentation is updated. - [x] I checked the diff, changed filenames, and commit messages for credentials, private data, internal URLs, internal issue identifiers, and generated artifacts. - [x] I recorded the validation commands and results above. - [x] I declared version bumps in the release-intent block above. `services/versions.json` is written by automation — do not edit it by hand. --------- Signed-off-by: Chris Kelsey <ckelsey@nvidia.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changelog title
Safer engine removal and data resets
Changelog body
~/.lmstudio/modelsis kept when LM Studio is removed.Bumps
Summary
Review fixes for the work below this in the stack. Stacked on
fix/indeterminate-install-progress(#145), at the top of #147 → #148 → #117 →#150 → #145; merge it after them, and before the stack ships.
Engine removal (#150)
as an irregular file, not a symlink, so the symlink check let the elevated
uninstaller read through one and empty the directory it names. Removal now
descends only into what
Lstatreports as a real directory. Two Windowstests use real junctions; CI runs Go tests on Linux only, so they run on
Windows machines.
another folder under the removal target was deleted, and case handling
followed the OS rather than the filesystem.
os.SameFilereplaces both. Newtests cover a symlinked store, a store two levels down (the first to exercise
the recursion), and a store spelled in another case.
spec.mdnow states what removing an engine may delete.Resets (#150)
The TUI and desktop resets wiped app data even when an engine could not be
removed, deleting the only record that it was PAIR's. Both now stop first and
say which engine, or that the service is not running. The orchestrator gains its
first tests.
Smaller fixes
wording. The data path and firewall rule names keep the old name on purpose.
~/.lmstudio.Test plan
go vet(darwin, linux, windows) andgo test ./...innvpair-engine-managerandnvpair-tui.npm run typecheck,npm run lint,npm run test:unitfromdesktop/.TestUninstallTerminatesRunningInstancefails locally on macOS identically ondevelop. The Windows junction tests compile but did not run here.