Skip to content

feat: verify CLI protocol version on LS start [IDE-1652]#391

Open
nick-y-snyk wants to merge 4 commits into
mainfrom
feat/IDE-1652-cli-protocol-version-gate
Open

feat: verify CLI protocol version on LS start [IDE-1652]#391
nick-y-snyk wants to merge 4 commits into
mainfrom
feat/IDE-1652-cli-protocol-version-gate

Conversation

@nick-y-snyk
Copy link
Copy Markdown
Contributor

Description

Adds verifyCliProtocolVersion() called from SnykLanguageServer.start() after the CLI path is validated. If the CLI reports a protocol version that doesn't match REQUIRED_LS_PROTOCOL_VERSION, the plugin logs and shows a user-visible error and aborts LS startup. Non-parseable output is treated as non-fatal (logged, startup continues).

Matches the equivalent gate added in VSCode extension PR #733. Part of IDE-1652.

Checklist

Screenshots / GIFs

N/A — no UI change.

@nick-y-snyk nick-y-snyk requested a review from a team as a code owner May 15, 2026 11:20
@snyk-io
Copy link
Copy Markdown

snyk-io Bot commented May 15, 2026

Snyk checks have passed. No issues have been found so far.

Status Scan Engine Critical High Medium Low Total (0)
Open Source Security 0 0 0 0 0 issues
Licenses 0 0 0 0 0 issues
Code Security 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

@snyk-pr-review-bot

This comment has been minimized.

@nick-y-snyk nick-y-snyk changed the base branch from main to feat/IDE-1652 May 15, 2026 11:27
ProcessBuilder pb = new ProcessBuilder(cliPath, "language-server", "--protocolVersion");
pb.redirectErrorStream(true);
Process proc = pb.start();
String output = new String(proc.getInputStream().readAllBytes()).trim();
@snyk-pr-review-bot

This comment has been minimized.

@snyk-pr-review-bot

This comment has been minimized.

@snyk-pr-review-bot

This comment has been minimized.

@nick-y-snyk nick-y-snyk force-pushed the feat/IDE-1652-cli-protocol-version-gate branch from a650f99 to 0936ad9 Compare May 15, 2026 18:36
@snyk-pr-review-bot

This comment has been minimized.

@snyk-pr-review-bot

This comment has been minimized.

@nick-y-snyk nick-y-snyk changed the base branch from feat/IDE-1652 to main May 21, 2026 14:54
Adds verifyCliProtocolVersion() called from start() after path validation.
Aborts with IOException + user-visible message when CLI version doesn't match
REQUIRED_LS_PROTOCOL_VERSION. Non-fatal on unparseable output.
@nick-y-snyk nick-y-snyk force-pushed the feat/IDE-1652-cli-protocol-version-gate branch from 638f254 to fd0c365 Compare May 22, 2026 13:29
@snyk-pr-review-bot

This comment has been minimized.

@snyk-pr-review-bot

This comment has been minimized.

@acke
Copy link
Copy Markdown
Contributor

acke commented May 22, 2026

Would it be possible to add a smoke test that actually downloads a real Snyk CLI binary and exercises verifyCliProtocolVersion() against it end-to-end? The current tests stub the CLI with a temp shell script (and skip on Windows), so they don't catch the case where the real CLI's language-server --protocolVersion output format changes or the version-negotiation contract drifts between the plugin and the actual CLI release.

… CLI binary

Downloads the real CLI via LsDownloader and asserts verifyCliProtocolVersion()
is non-fatal when --protocolVersion flag is unknown (not yet in released builds).
@snyk-pr-review-bot
Copy link
Copy Markdown

PR Reviewer Guide 🔍

🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Indefinite Blocking 🟠 [major]

The verifyCliProtocolVersion method calls proc.getInputStream().readAllBytes() before proc.waitFor(). Since readAllBytes() blocks until the stream is closed or EOF is reached, if the CLI process hangs or fails to close its output stream, the Language Server startup thread will block indefinitely. The 5-second timeout on waitFor is never reached in this scenario. Consider reading from the stream with a timeout or using the process's output after the timeout has been enforced.

String output = new String(proc.getInputStream().readAllBytes(), StandardCharsets.UTF_8).trim();
proc.waitFor(5, TimeUnit.SECONDS);
📚 Repository Context Analyzed

This review considered 8 relevant code sections from 7 files (average relevance: 0.79)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants