Follow-up from the comprehensive review of PR #239 (LSP server). Line numbers reference the PR branch at review time.
Severity: major (1), minor (2), nit (3)
-
Doctor verifies the wrong location for inference-lsp (doctor.rs:364): the check tests toolchains/<default>/inference-lsp, but the VS Code resolve chain only uses <INFERENCE_HOME>/bin/inference-lsp (resolve.ts:68) and PATH (whose managed entry is also bin/). States where the binary exists in the toolchain dir but bin/ has no lsp symlink are reachable and likely during rollout:
- a pre-PR
infs installs a toolchain archive (extraction unpacks inference-lsp; old update_symlinks links only infc); the user then upgrades infs — nothing re-runs update_symlinks;
- new
infs install of an already-installed version early-returns without update_symlinks when a default is already set (commands/install.rs).
In both states infs doctor prints [OK] inference-lsp: Found at .../toolchains/<v>/inference-lsp while the extension fails with "inference-lsp not found (searched .../bin and PATH)". The natural remediation infs install is a no-op ("already installed"); only the non-obvious infs default <v> heals it. Fix: additionally verify paths.symlink_path(binary_with_ext) resolves; if the toolchain bundles the server but the bin/ link is missing/broken, emit a Warning telling the user to run infs default <version> (or repair the link automatically).
-
The "also on PATH" note reports the install's own symlink (doctor.rs:339): infs puts <INFERENCE_HOME>/bin on PATH at first install, and the extension's runDoctor prepends it explicitly — so on every healthy install which("inference-lsp") resolves to the managed symlink itself and the OK line implies a duplicate copy exists. Exclude the managed bin dir from the PATH-copy note.
-
check_inference_lsp hardcodes the binary name (doctor.rs:338 etc.) instead of iterating ToolchainPaths::OPTIONAL_MANAGED_BINARIES; a second optional managed binary would silently get no doctor coverage.
Follow-up from the comprehensive review of PR #239 (LSP server). Line numbers reference the PR branch at review time.
Severity: major (1), minor (2), nit (3)
Doctor verifies the wrong location for inference-lsp (
doctor.rs:364): the check teststoolchains/<default>/inference-lsp, but the VS Code resolve chain only uses<INFERENCE_HOME>/bin/inference-lsp(resolve.ts:68) and PATH (whose managed entry is alsobin/). States where the binary exists in the toolchain dir butbin/has no lsp symlink are reachable and likely during rollout:infsinstalls a toolchain archive (extraction unpacks inference-lsp; oldupdate_symlinkslinks only infc); the user then upgrades infs — nothing re-runsupdate_symlinks;infs installof an already-installed version early-returns withoutupdate_symlinkswhen a default is already set (commands/install.rs).In both states
infs doctorprints[OK] inference-lsp: Found at .../toolchains/<v>/inference-lspwhile the extension fails with "inference-lsp not found (searched .../bin and PATH)". The natural remediationinfs installis a no-op ("already installed"); only the non-obviousinfs default <v>heals it. Fix: additionally verifypaths.symlink_path(binary_with_ext)resolves; if the toolchain bundles the server but the bin/ link is missing/broken, emit a Warning telling the user to runinfs default <version>(or repair the link automatically).The "also on PATH" note reports the install's own symlink (
doctor.rs:339):infsputs<INFERENCE_HOME>/binon PATH at first install, and the extension'srunDoctorprepends it explicitly — so on every healthy installwhich("inference-lsp")resolves to the managed symlink itself and the OK line implies a duplicate copy exists. Exclude the managed bin dir from the PATH-copy note.check_inference_lsphardcodes the binary name (doctor.rs:338etc.) instead of iteratingToolchainPaths::OPTIONAL_MANAGED_BINARIES; a second optional managed binary would silently get no doctor coverage.