Auto-install iii-lsp binary in VS Code extension#25
Merged
Conversation
📝 WalkthroughWalkthroughThe PR introduces automated binary provisioning for the iii-lsp VS Code extension via a Docker-based build environment. It adds an installer module that downloads, verifies, and caches the server binary on first activation, includes comprehensive test coverage, updates the CI workflow to run tests and package checks, and provides Make targets for local development with container orchestration. Changes
Sequence DiagramsequenceDiagram
participant User
participant VSCodeExt as VS Code Extension
participant Installer
participant GitHubReleases as GitHub Releases
participant Storage as Global Storage
participant Filesystem
User->>VSCodeExt: Activate extension
VSCodeExt->>Installer: ensureServerBinary(context)
Installer->>Installer: Check iii-lsp.serverPath config
alt Config path exists
Installer->>Filesystem: Verify binary executable
Filesystem-->>Installer: Path valid
Installer-->>VSCodeExt: Return configured path
else Config path missing
Installer->>Storage: Check cached binary version
alt Binary cached
Installer->>Filesystem: Verify cached binary
Filesystem-->>Installer: Binary exists and executable
Installer->>VSCodeExt: config.update(serverPath)
Installer-->>VSCodeExt: Return cached path
else Binary not cached
Installer->>GitHubReleases: Fetch checksum
GitHubReleases-->>Installer: SHA256 hash
Installer->>GitHubReleases: Download archive
GitHubReleases-->>Installer: Binary archive
Installer->>Installer: Verify archive checksum
Installer->>Filesystem: Extract to versioned dir
Filesystem-->>Installer: Binary extracted
Installer->>VSCodeExt: config.update(serverPath)
Installer-->>VSCodeExt: Return installed path
end
end
VSCodeExt->>VSCodeExt: Initialize language server with resolved path
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
sergiofilhowz
approved these changes
Apr 13, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
112-113: Use the containerized Make targets in CI for environment parity.Line 112 and Line 116 execute Node tasks directly on the host runner. Consider switching these to
make install,make test, andmake package-checkso CI matches the project’s containerized workflow.♻️ Suggested CI adjustment
- - uses: actions/setup-node@v4 - with: - node-version: 20 - - - name: Install dependencies - run: npm ci + - name: Install dependencies (containerized) + run: make install - name: Run tests - run: npm test + run: make test - name: Package check (dry run) - run: npm run package:check + run: make package-checkBased on learnings: Use containers for Node commands in the iii-lsp-vscode project. Do not install host-global Node packages.
Also applies to: 116-116
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 112 - 113, Replace direct host Node invocations with the repository Make targets to run inside the project container: update the CI job steps that currently run "npm test" (step named "Run tests") and any "npm ci" or host Node install/pack commands to call "make test", "make install" (or "make deps" if present), and "make package-check" respectively so the workflow uses the containerized environment; locate the steps using the command strings "npm test" and "npm ci"/"npm install" in the workflow and swap them to the corresponding "make" targets.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@iii-lsp-vscode/Dockerfile`:
- Around line 1-3: The image currently runs as root; update the Dockerfile to
create or use a non-root user and switch to it with a USER directive: add steps
after WORKDIR to create a dedicated user/group (or use the existing node user),
ensure /workspace is owned by that user (chown) and any required file
permissions are set, and then add USER <username> so the container runs non-root
by default; reference the existing WORKDIR /workspace and the Dockerfile's USER
directive when making the change.
In `@iii-lsp-vscode/installer.js`:
- Around line 149-165: extractArchive currently writes and extracts directly
into installDir using a fixed archivePath under os.tmpdir(), which causes races
across concurrent extension hosts; change extractArchive (and any callers at
lines noted) to download/extract into a unique staging directory (e.g., use
fs.mkdtemp or include SERVER_VERSION + process.pid/UUID in the temp name),
perform checksum/verification inside that staging dir, then atomically rename
(fs.rename) the staged binary/dir into the final installDir; also acquire a
simple filesystem lock (flock or a lockfile) around the rename/installation step
to serialize installations, and ensure proper cleanup of the staging dir on
error so concurrent processes do not clobber each other.
- Around line 18-27: PLATFORM_TARGETS currently maps Linux keys to glibc/armv7
binaries which breaks on musl/armv6 hosts; update the installer logic that uses
PLATFORM_TARGETS to detect libc and CPU ABI on Linux before selecting an asset:
run a libc detection (e.g., inspect ldd --version or read /etc/os-release or use
process.report/execSync) and check ARM ABI (armv6 vs armv7 vs arm64) and if the
host is musl or an unsupported ABI, throw an UnsupportedError (or return/throw a
sentinel) instead of selecting a GNU binary so the higher-level installer can
fall back to PATH; adjust any code that indexes PLATFORM_TARGETS (the mapping
object and its consumers) to only pick entries for verified libc/ABI combos or
propagate the Unsupported signal.
- Around line 84-111: The fetchResponse function uses httpsModule.get to create
a request that never times out; to prevent indefinite hangs, add a 30_000ms hard
timeout on the created request (the request variable returned by httpsModule.get
inside fetchResponse) by calling request.setTimeout(30000, () =>
request.destroy(new Error(`Timed out fetching ${url}`))); immediately after the
request is created so stalled downloads are rejected and the Promise's reject
handler runs.
In `@iii-lsp-vscode/Makefile`:
- Around line 15-19: The Makefile targets test and package-check can run on a
clean checkout without installing dependencies; update the test and
package-check targets (and/or add a deps target) so they ensure npm dependencies
are installed before running commands—for example, run $(DOCKER_RUN) npm ci (or
add a deps target that runs $(DOCKER_RUN) npm ci and make test and package-check
depend on deps) prior to invoking $(DOCKER_RUN) npm test and $(DOCKER_RUN) npm
run package:check respectively.
In `@iii-lsp-vscode/test/installer.test.js`:
- Around line 127-142: The test "fileExists requires a regular executable binary
on non-win32" should be skipped on Windows: at the start of the test function
(before creating tempDir/tempFile), add a guard that returns early when
process.platform === 'win32' (or call this.skip()/return) so the rest of the
logic for fileExists and fs.chmod(0o755) only runs on non-win32; update the test
body around the existing test and references to fileExists, tempFile, and
tempDir accordingly.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 112-113: Replace direct host Node invocations with the repository
Make targets to run inside the project container: update the CI job steps that
currently run "npm test" (step named "Run tests") and any "npm ci" or host Node
install/pack commands to call "make test", "make install" (or "make deps" if
present), and "make package-check" respectively so the workflow uses the
containerized environment; locate the steps using the command strings "npm test"
and "npm ci"/"npm install" in the workflow and swap them to the corresponding
"make" targets.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 014010bd-f634-42bc-a307-7cb750062b9b
⛔ Files ignored due to path filters (2)
iii-lsp-vscode/lsp.gifis excluded by!**/*.gifiii-lsp-vscode/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (9)
.github/workflows/ci.ymliii-lsp-vscode/AGENTS.mdiii-lsp-vscode/Dockerfileiii-lsp-vscode/Makefileiii-lsp-vscode/README.mdiii-lsp-vscode/extension.jsiii-lsp-vscode/installer.jsiii-lsp-vscode/package.jsoniii-lsp-vscode/test/installer.test.js
Comment on lines
+1
to
+3
| FROM node:20-bookworm | ||
|
|
||
| WORKDIR /workspace |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify whether a non-root USER is set in the Dockerfile.
rg -n '^\s*USER\s+' iii-lsp-vscode/Dockerfile || trueRepository: iii-hq/workers
Length of output: 40
Add a non-root default user.
The Dockerfile currently defaults to the root user. Add a USER directive to run containers as a non-root user by default.
Proposed fix
FROM node:20-bookworm
WORKDIR /workspace
+USER node📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| FROM node:20-bookworm | |
| WORKDIR /workspace | |
| FROM node:20-bookworm | |
| WORKDIR /workspace | |
| USER node |
🧰 Tools
🪛 Trivy (0.69.3)
[error] 1-1: Image user should not be 'root'
Specify at least 1 USER command in Dockerfile with non-root user as argument
Rule: DS-0002
(IaC/Dockerfile)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@iii-lsp-vscode/Dockerfile` around lines 1 - 3, The image currently runs as
root; update the Dockerfile to create or use a non-root user and switch to it
with a USER directive: add steps after WORKDIR to create a dedicated user/group
(or use the existing node user), ensure /workspace is owned by that user (chown)
and any required file permissions are set, and then add USER <username> so the
container runs non-root by default; reference the existing WORKDIR /workspace
and the Dockerfile's USER directive when making the change.
Comment on lines
+18
to
+27
| const PLATFORM_TARGETS = { | ||
| 'darwin/arm64': 'aarch64-apple-darwin', | ||
| 'darwin/x64': 'x86_64-apple-darwin', | ||
| 'linux/arm64': 'aarch64-unknown-linux-gnu', | ||
| 'linux/arm': 'armv7-unknown-linux-gnueabihf', | ||
| 'linux/x64': 'x86_64-unknown-linux-gnu', | ||
| 'win32/arm64': 'aarch64-pc-windows-msvc', | ||
| 'win32/ia32': 'i686-pc-windows-msvc', | ||
| 'win32/x64': 'x86_64-pc-windows-msvc', | ||
| }; |
There was a problem hiding this comment.
Linux target selection needs libc / ABI detection.
process.arch is not enough on Linux. These mappings always pick GNU/glibc assets, and linux/arm also assumes armv7 hard-float. Alpine/musl or armv6 hosts will download a binary that cannot start, and this code will then cache that broken path. Detect the libc/ABI first and throw Unsupported so the installer failure path can fall back to PATH, or publish/select matching assets.
Also applies to: 29-38
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@iii-lsp-vscode/installer.js` around lines 18 - 27, PLATFORM_TARGETS currently
maps Linux keys to glibc/armv7 binaries which breaks on musl/armv6 hosts; update
the installer logic that uses PLATFORM_TARGETS to detect libc and CPU ABI on
Linux before selecting an asset: run a libc detection (e.g., inspect ldd
--version or read /etc/os-release or use process.report/execSync) and check ARM
ABI (armv6 vs armv7 vs arm64) and if the host is musl or an unsupported ABI,
throw an UnsupportedError (or return/throw a sentinel) instead of selecting a
GNU binary so the higher-level installer can fall back to PATH; adjust any code
that indexes PLATFORM_TARGETS (the mapping object and its consumers) to only
pick entries for verified libc/ABI combos or propagate the Unsupported signal.
Comment on lines
+84
to
+111
| function fetchResponse(url, httpsModule, redirectsRemaining) { | ||
| return new Promise((resolve, reject) => { | ||
| const request = httpsModule.get(url, (response) => { | ||
| const { statusCode, headers } = response; | ||
|
|
||
| if (statusCode >= 300 && statusCode < 400 && headers.location) { | ||
| response.resume(); | ||
|
|
||
| if (redirectsRemaining <= 0) { | ||
| reject(new Error(`Too many redirects while fetching ${url}`)); | ||
| return; | ||
| } | ||
|
|
||
| resolve(fetchResponse(new URL(headers.location, url).href, httpsModule, redirectsRemaining - 1)); | ||
| return; | ||
| } | ||
|
|
||
| if (statusCode !== 200) { | ||
| response.resume(); | ||
| reject(new Error(`Unexpected status code ${statusCode} for ${url}`)); | ||
| return; | ||
| } | ||
|
|
||
| resolve(response); | ||
| }); | ||
|
|
||
| request.on('error', reject); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
wc -l iii-lsp-vscode/installer.jsRepository: iii-hq/workers
Length of output: 89
🏁 Script executed:
cat -n iii-lsp-vscode/installer.jsRepository: iii-hq/workers
Length of output: 9061
Add a hard timeout to these HTTPS requests.
https.get() at line 86 has no deadline. If GitHub or the network stalls, the ensureServerBinary() function—called during activation—will hang indefinitely, blocking extension startup. The fallback configured path (checked at line 194) only works if a binary path is already configured; on first install, the download is required.
Add request.setTimeout(30_000, () => { request.destroy(new Error(Timed out fetching ${url})); }); after line 86 to reject stalled requests.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@iii-lsp-vscode/installer.js` around lines 84 - 111, The fetchResponse
function uses httpsModule.get to create a request that never times out; to
prevent indefinite hangs, add a 30_000ms hard timeout on the created request
(the request variable returned by httpsModule.get inside fetchResponse) by
calling request.setTimeout(30000, () => request.destroy(new Error(`Timed out
fetching ${url}`))); immediately after the request is created so stalled
downloads are rejected and the Promise's reject handler runs.
Comment on lines
+149
to
+165
| async function extractArchive(archivePath, installDir, platform = process.platform) { | ||
| await fsp.rm(installDir, { recursive: true, force: true }); | ||
| await fsp.mkdir(installDir, { recursive: true }); | ||
|
|
||
| if (platform === 'win32') { | ||
| await extractZip(archivePath, { dir: installDir }); | ||
| } else { | ||
| await tar.x({ file: archivePath, cwd: installDir }); | ||
| } | ||
|
|
||
| const binaryPath = path.join(installDir, getBinaryName(platform)); | ||
|
|
||
| if (platform !== 'win32') { | ||
| await fsp.chmod(binaryPath, 0o755); | ||
| } | ||
|
|
||
| return binaryPath; |
There was a problem hiding this comment.
Serialize installation; the current staging paths race across extension hosts.
archivePath is a fixed filename under os.tmpdir() and even omits SERVER_VERSION, while extractArchive() clears the final installDir in place. Two windows activating together can overwrite each other’s download, fail checksum verification, or delete the other process’s extracted binary. Stage into a unique temp dir under a lock, then atomically rename into installDir.
Also applies to: 168-179, 198-217
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@iii-lsp-vscode/installer.js` around lines 149 - 165, extractArchive currently
writes and extracts directly into installDir using a fixed archivePath under
os.tmpdir(), which causes races across concurrent extension hosts; change
extractArchive (and any callers at lines noted) to download/extract into a
unique staging directory (e.g., use fs.mkdtemp or include SERVER_VERSION +
process.pid/UUID in the temp name), perform checksum/verification inside that
staging dir, then atomically rename (fs.rename) the staged binary/dir into the
final installDir; also acquire a simple filesystem lock (flock or a lockfile)
around the rename/installation step to serialize installations, and ensure
proper cleanup of the staging dir on error so concurrent processes do not
clobber each other.
Comment on lines
+15
to
+19
| test: image | ||
| $(DOCKER_RUN) npm test | ||
|
|
||
| package-check: image | ||
| $(DOCKER_RUN) npm run package:check |
There was a problem hiding this comment.
test and package-check can fail on a clean checkout.
Line 15-Line 19 don’t ensure dependencies are installed before running npm commands.
🔧 Proposed fix
-test: image
+test: install
$(DOCKER_RUN) npm test
-package-check: image
+package-check: install
$(DOCKER_RUN) npm run package:check🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@iii-lsp-vscode/Makefile` around lines 15 - 19, The Makefile targets test and
package-check can run on a clean checkout without installing dependencies;
update the test and package-check targets (and/or add a deps target) so they
ensure npm dependencies are installed before running commands—for example, run
$(DOCKER_RUN) npm ci (or add a deps target that runs $(DOCKER_RUN) npm ci and
make test and package-check depend on deps) prior to invoking $(DOCKER_RUN) npm
test and $(DOCKER_RUN) npm run package:check respectively.
Comment on lines
+127
to
+142
| test('fileExists requires a regular executable binary on non-win32', async () => { | ||
| const tempDir = await fs.mkdtemp(path.join(os.tmpdir(), 'iii-lsp-vscode-test-')); | ||
| const tempFile = path.join(tempDir, 'iii-lsp'); | ||
|
|
||
| await fs.mkdir(path.join(tempDir, 'subdir'), { recursive: true }); | ||
| await fs.writeFile(tempFile, 'fake binary'); | ||
|
|
||
| assert.equal(await fileExists(path.join(tempDir, 'subdir')), false); | ||
| assert.equal(await fileExists(tempFile), false); | ||
|
|
||
| if (process.platform !== 'win32') { | ||
| await fs.chmod(tempFile, 0o755); | ||
| } | ||
|
|
||
| assert.equal(await fileExists(tempFile), true); | ||
| }); |
There was a problem hiding this comment.
Make this test explicitly skip on Windows.
The test title says “non-win32”, but it still runs on Windows. Add an early return at the top to avoid platform-dependent failures.
🔧 Proposed fix
test('fileExists requires a regular executable binary on non-win32', async () => {
+ if (process.platform === 'win32') {
+ return;
+ }
+
const tempDir = await fs.mkdtemp(path.join(os.tmpdir(), 'iii-lsp-vscode-test-'));
const tempFile = path.join(tempDir, 'iii-lsp');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| test('fileExists requires a regular executable binary on non-win32', async () => { | |
| const tempDir = await fs.mkdtemp(path.join(os.tmpdir(), 'iii-lsp-vscode-test-')); | |
| const tempFile = path.join(tempDir, 'iii-lsp'); | |
| await fs.mkdir(path.join(tempDir, 'subdir'), { recursive: true }); | |
| await fs.writeFile(tempFile, 'fake binary'); | |
| assert.equal(await fileExists(path.join(tempDir, 'subdir')), false); | |
| assert.equal(await fileExists(tempFile), false); | |
| if (process.platform !== 'win32') { | |
| await fs.chmod(tempFile, 0o755); | |
| } | |
| assert.equal(await fileExists(tempFile), true); | |
| }); | |
| test('fileExists requires a regular executable binary on non-win32', async () => { | |
| if (process.platform === 'win32') { | |
| return; | |
| } | |
| const tempDir = await fs.mkdtemp(path.join(os.tmpdir(), 'iii-lsp-vscode-test-')); | |
| const tempFile = path.join(tempDir, 'iii-lsp'); | |
| await fs.mkdir(path.join(tempDir, 'subdir'), { recursive: true }); | |
| await fs.writeFile(tempFile, 'fake binary'); | |
| assert.equal(await fileExists(path.join(tempDir, 'subdir')), false); | |
| assert.equal(await fileExists(tempFile), false); | |
| if (process.platform !== 'win32') { | |
| await fs.chmod(tempFile, 0o755); | |
| } | |
| assert.equal(await fileExists(tempFile), true); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@iii-lsp-vscode/test/installer.test.js` around lines 127 - 142, The test
"fileExists requires a regular executable binary on non-win32" should be skipped
on Windows: at the start of the test function (before creating
tempDir/tempFile), add a guard that returns early when process.platform ===
'win32' (or call this.skip()/return) so the rest of the logic for fileExists and
fs.chmod(0o755) only runs on non-win32; update the test body around the existing
test and references to fileExists, tempFile, and tempDir accordingly.
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.
Summary
iii-lsp/v0.1.0binary, verifies its SHA-256 checksum, installs it under extension global storage, and savesiii-lsp.serverPath.iii-lsponPATH.Validation
docker run --rm -u "$(id -u):$(id -g)" -e npm_config_cache=/tmp/npm-cache -v "$PWD:/workspace" -w /workspace iii-lsp-vscode-dev npm cidocker run --rm -u "$(id -u):$(id -g)" -e npm_config_cache=/tmp/npm-cache -v "$PWD:/workspace" -w /workspace iii-lsp-vscode-dev npm testdocker run --rm -u "$(id -u):$(id -g)" -e npm_config_cache=/tmp/npm-cache -v "$PWD:/workspace" -w /workspace iii-lsp-vscode-dev npm run package:checkaarch64-apple-darwinbinary, wroteiii-lsp.serverPath, and the installed binary ran--help.Summary by CodeRabbit
Release Notes
New Features
Documentation
Chores