Repository navigation
fix(lsp): check a project-local pyright-langserver before PATH and bunx - #148
Conversation
Resolve pyright-langserver from node_modules/.bin, walking up from the server root, before falling back to PATH and the pinned bunx run. This makes the pyright server honor the version a project already installs. Move findPackageBin and resolvePackageBin from typescript.ts into utils.ts so both servers share them. Closes #147
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the package binary resolution logic (resolvePackageBin and findPackageBin) from typescript.ts to utils.ts to make it reusable, and integrates it into pyright.ts to support resolving and spawning a project-local pyright-langserver. Feedback was provided regarding a Windows compatibility issue where spawning a .cmd shim resolved by findPackageBin on Windows will fail without shell: true, along with a recommendation to wrap the spawn call in a try/catch block to handle OS-level errors.
 [Medium impact] The PR appears safe to merge; the latest refactor preserves language-server startup behavior. SummaryPyright now prefers a project-local server, including one installed above a nested Python project.
Reviews (3) · Last reviewed commit: "refactor(lsp): share Windows shim spawn ..." · Reviewed by Greptile |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
cubic analysis
All reported issues were addressed across 4 files
Linked issue analysis
Linked issue: pleaseai/code-intelligence#147: fix(lsp): pyright server ignores a project-local pyright-langserver in node_modules/.bin
| Status | Acceptance criteria | Notes |
|---|---|---|
| ✅ | Prefer the project-local Pyright binary before PATH and bunx | PyrightServer calls findPackageBin first, then checks PATH, leaving bunx as the fallback. |
| ✅ | Find ancestor node_modules when the Python root is nested | findPackageBin walks from the supplied root to its ancestors, and a test covers a nested backend root. |
| ✅ | Accept only a shim owned by the pyright package | resolvePackageBin verifies the shim against the package target, and a test confirms stray shims are ignored. |
| ✅ | Share package-bin resolution helpers from utils.ts | Both helpers are exported from utils.ts and imported by TypeScript and Pyright servers. |
| ✅ | Do not probe pyright-langserver with a version command | Pyright uses package lookup and PATH selection directly, with no version probe before spawning. |
Architecture diagram
sequenceDiagram
participant Editor as Editor Client
participant PyrightSrv as PyrightServer.spawn(root)
participant Utils as utils.ts
participant FSBin as findPackageBin(root, 'pyright', 'pyright-langserver')
participant FSResolve as resolvePackageBin(packageRoot, name)
participant FSPath as node:fs
participant PATH as PATH lookup
participant BunX as bunx fallback
participant Proc as Spawned process
Note over PyrightSrv,BunX: Binary resolution precedence for pyright-langserver
Editor->>PyrightSrv: spawn(root)
PyrightSrv->>FSBin: findPackageBin(root, 'pyright', 'pyright-langserver')
activate FSBin
loop Walk up from root to filesystem root
FSBin->>FSBin: Check node_modules/pyright/ in current dir
FSBin->>FSResolve: resolvePackageBin(packageRoot, 'pyright-langserver')
activate FSResolve
FSResolve->>FSPath: read package.json
FSPath-->>FSResolve: bin field
alt package declares pyright-langserver bin
FSResolve->>FSPath: Check target exists
FSResolve->>FSPath: Check .bin/pyright-langserver shim exists
alt Shim is symlink
FSResolve->>FSPath: Resolve shim target
alt Resolved target matches package target
FSResolve-->>FSBin: Return shim path
else Mismatch - stale/unrelated shim
FSResolve-->>FSBin: Return undefined
end
else Shim is wrapper script
FSResolve->>FSPath: Read wrapper content
alt Wrapper references package target
FSResolve-->>FSBin: Return shim path
else Unrelated wrapper
FSResolve-->>FSBin: Return undefined
end
end
else No matching bin declared
FSResolve-->>FSBin: Return undefined
end
deactivate FSResolve
alt Bin found in node_modules/.bin
FSBin-->>PyrightSrv: Return project-local binary path
else Not found - continue up
FSBin->>FSBin: Move to parent directory
end
end
deactivate FSBin
alt Project-local binary found
PyrightSrv->>Proc: spawn(local-pyright-langserver, --stdio)
else No project-local binary
PyrightSrv->>PATH: Bun.which('pyright-langserver', { PATH })
alt Found on PATH
PyrightSrv->>Proc: spawn(path-pyright-langserver, --stdio)
else Not on PATH
PyrightSrv->>BunX: bunx pyright-langserver --stdio
BunX->>Proc: Spawn via bunx
end
end
Proc-->>PyrightSrv: LSP process
PyrightSrv-->>Editor: LSP server handle
Reply with feedback, questions, or to request a fix.
Turn on auto-fix | Re-trigger cubic
…m shims On Windows a project-local or npm-global pyright-langserver is a .cmd shim, which spawn can only run through a shell, so reuse nativeServerSpawnArgs as the classic typescript path does. pnpm .bin wrappers reference the package's real path in the .pnpm store, so resolvePackageBin also matches the wrapper against the realpath of the target.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors how language server binaries are located and spawned, moving the resolvePackageBin and findPackageBin utilities to a shared utils.ts module and adding support for pnpm wrapper shims. It also updates the Pyright server to use these utilities and adds corresponding unit tests. The review feedback highlights a tight coupling issue where pyright.ts directly imports nativeServerSpawnArgs from the TypeScript module. To resolve this dependency disorder, the reviewer suggests extracting the Windows command preparation logic into a shared prepareSpawnCommand utility in utils.ts.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
pyright no longer imports nativeServerSpawnArgs from the typescript server. Both servers use prepareSpawnCommand from utils, and the classic typescript fallback calls it directly since it never used the native --lsp args.
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
|
/gemini review |
|
There was a problem hiding this comment.
Code Review
This pull request refactors LSP server binary resolution by moving shared utility functions like 'resolvePackageBin' and 'findPackageBin' from 'typescript.ts' to 'utils.ts', and introduces 'prepareSpawnCommand' to handle Windows shell execution consistently. It also adds support for resolving pnpm wrapper shims pointing to the '.pnpm' store, along with comprehensive unit tests for Pyright fallback scenarios. Feedback on the changes suggests extracting the inline helper 'normalizeShimPath' inside 'resolvePackageBin' to the module level to reduce cognitive load and comply with the organization's 50 lines-of-code limit for functions.
| export function resolvePackageBin(packageRoot: string, name: string): string | undefined { | ||
| try { | ||
| const pkg = JSON.parse( | ||
| readFileSync(path.join(packageRoot, 'package.json'), 'utf8'), | ||
| ) as { bin?: string | Record<string, string> } | ||
| const relativeTarget = typeof pkg.bin === 'string' ? pkg.bin : pkg.bin?.[name] | ||
| if (!relativeTarget) { return undefined } | ||
|
|
||
| const target = path.resolve(packageRoot, relativeTarget) | ||
| if (!existsSync(target)) { return undefined } | ||
| const ext = process.platform === 'win32' ? '.cmd' : '' | ||
| const packageDir = path.dirname(packageRoot) | ||
| const nodeModules = path.basename(packageDir).startsWith('@') | ||
| ? path.dirname(packageDir) | ||
| : packageDir | ||
| const shim = path.join(nodeModules, '.bin', `${name}${ext}`) | ||
| if (!existsSync(shim)) { return undefined } | ||
|
|
||
| // npm-style symlinks and package-manager-generated wrapper scripts are both | ||
| // valid shims. For wrappers, require a reference to the package's declared | ||
| // target so a stale or unrelated shared shim is never selected. | ||
| const stat = lstatSync(shim) | ||
| if (stat.isSymbolicLink()) { | ||
| const resolvedShim = path.resolve(path.dirname(shim), readlinkSync(shim)) | ||
| return existsSync(resolvedShim) | ||
| && realpathSync(resolvedShim) === realpathSync(target) | ||
| ? shim | ||
| : undefined | ||
| } | ||
| const normalizeShimPath = (value: string): string => { | ||
| const normalized = value.replaceAll('\\', '/') | ||
| return process.platform === 'win32' ? normalized.toLowerCase() : normalized | ||
| } | ||
| const wrapper = normalizeShimPath(readFileSync(shim, 'utf8')) | ||
| // pnpm wrappers reference the package's real path in its .pnpm store, not the node_modules link. | ||
| const binDir = path.dirname(shim) | ||
| const realTarget = realpathSync(target) | ||
| const references = [ | ||
| path.relative(binDir, target), | ||
| target, | ||
| path.relative(realpathSync(binDir), realTarget), | ||
| realTarget, | ||
| ].map(normalizeShimPath) | ||
| return references.some(reference => wrapper.includes(reference)) | ||
| ? shim | ||
| : undefined | ||
| } | ||
| catch { | ||
| return undefined | ||
| } | ||
| } |
There was a problem hiding this comment.
[MEDIUM] Cognitive Overload: Extract inline helper to module level
Symptom — The resolvePackageBin function is 51 lines long, exceeding the organization's function length limit of 50 lines, and contains an inline helper function normalizeShimPath.
Source — Fowler — Refactoring (Long Method) / Organization Style Guide (Section 2, Code Limits).
Consequence — Having inline helper functions inside a relatively long method increases cognitive load, making the code harder to read, maintain, and test in isolation.
Remedy — Extract normalizeShimPath to the module level, which reduces the length of resolvePackageBin to 46 lines, keeping it under the 50 LOC limit.
const normalizeShimPath = (value: string): string => {
const normalized = value.replaceAll('\\', '/')
return process.platform === 'win32' ? normalized.toLowerCase() : normalized
}
/** Resolve a package-owned bin declared by the package and exposed by its shared shim. */
export function resolvePackageBin(packageRoot: string, name: string): string | undefined {
try {
const pkg = JSON.parse(
readFileSync(path.join(packageRoot, 'package.json'), 'utf8'),
) as { bin?: string | Record<string, string> }
const relativeTarget = typeof pkg.bin === 'string' ? pkg.bin : pkg.bin?.[name]
if (!relativeTarget) { return undefined }
const target = path.resolve(packageRoot, relativeTarget)
if (!existsSync(target)) { return undefined }
const ext = process.platform === 'win32' ? '.cmd' : ''
const packageDir = path.dirname(packageRoot)
const nodeModules = path.basename(packageDir).startsWith('@')
? path.dirname(packageDir)
: packageDir
const shim = path.join(nodeModules, '.bin', `${name}${ext}`)
if (!existsSync(shim)) { return undefined }
// npm-style symlinks and package-manager-generated wrapper scripts are both
// valid shims. For wrappers, require a reference to the package's declared
// target so a stale or unrelated shared shim is never selected.
const stat = lstatSync(shim)
if (stat.isSymbolicLink()) {
const resolvedShim = path.resolve(path.dirname(shim), readlinkSync(shim))
return existsSync(resolvedShim)
&& realpathSync(resolvedShim) === realpathSync(target)
? shim
: undefined
}
const wrapper = normalizeShimPath(readFileSync(shim, 'utf8'))
// pnpm wrappers reference the package's real path in its .pnpm store, not the node_modules link.
const binDir = path.dirname(shim)
const realTarget = realpathSync(target)
const references = [
path.relative(binDir, target),
target,
path.relative(realpathSync(binDir), realTarget),
realTarget,
].map(normalizeShimPath)
return references.some(reference => wrapper.includes(reference))
? shim
: undefined
}
catch {
return undefined
}
}References
- Function length must be less than or equal to 50 lines of code. Extract helper functions to adhere to this limit. (link)
Summary
PyrightServeronly looked at PATH and then the pinned bunx fallback, so apyright-langserverinstalled in the project'snode_moduleswas ignored. It now checks the project-local binary first, the same way the TypeScript server does.pyright.tsresolvesfindPackageBin(root, 'pyright', 'pyright-langserver') ?? Bun.which('pyright-langserver', { PATH }), then the pinned bunx fallback. The lookup walks up fromroot, because a Python root (pyproject.tomland similar) often sits in a subdirectory below the repo'snode_modules.findPackageBinandresolvePackageBinmove verbatim fromtypescript.tsintoutils.tsand are exported;typescript.tsimports them. No behavior change for TypeScript.packages/lsp/test/unit/server.test.ts:spawnServerWithgains an optionalsubdirparam, plus threePyrightServerfallback tests (project-local wins over PATH and bunx, anode_modulesin a parent of the root is found, a.bin/pyright-langservershim the pyright package does not own is ignored).Scope
The check is existence and ownership only.
pyright-langserverhas no--version, so there is norunsVersionprobe. A broken mise shim on PATH is out of scope, since it needs a probe other than--version.Verification
bun test test/unitinpackages/lsp: 179 pass, 0 fail.pyright.ts. The ownership test passes on the old code too, so it is a regression guard, not proof.bunx tsc --noEmit -p tsconfig.jsoninpackages/lsp: exit 0. eslint on the changed files: clean.Related issue
Closes #147
Checklist
bun test test/unitinpackages/lsp)BREAKING CHANGE:note is includedSummary by cubic
Fixes the Pyright server so a project-local
pyright-langserverundernode_modulesis used before PATH and the pinned bunx run, making editor diagnostics match the version the project pins.findPackageBinandresolvePackageBinmove fromtypescript.tsintoutils.tsand are shared; the TypeScript server behavior is unchanged, and.binshims thepyrightpackage doesn't own are ignored.node_modulesat the repository root..cmdshims now run through a shell via the sharedprepareSpawnCommand, and pnpm.binwrappers are accepted by matching against the package's real path in the.pnpmstore.Written for commit 0486614. Summary will update on new commits.