feat: verify CLI protocol version on LS start [IDE-1652]#391
Open
nick-y-snyk wants to merge 34 commits into
Open
feat: verify CLI protocol version on LS start [IDE-1652]#391nick-y-snyk wants to merge 34 commits into
nick-y-snyk wants to merge 34 commits into
Conversation
…nfig management [IDE-1652] Implement Story 1.1: Configuration reception, storage, and persistence. Adds new message types (ConfigSetting, LspFolderConfig, LspConfigurationParam), FolderConfigSettings singleton for thread-safe folder config storage, and snykConfiguration handler on SnykExtendedLanguageClient that persists global settings and manages folder configs from the Language Server.
…[IDE-1652] - Add explicitChanges tracking to Preferences (mark/check/clear/persist with flush) - Refactor LsConfigurationUpdater to produce LspConfigurationParam instead of old Settings type - Add ConfigSetting.outbound() factory for value+changed only outbound settings - Update SnykLanguageServer.getInitializationOptions() to use buildConfigurationParam() - Add convertInboundValue() for scanning mode format conversion - Synchronize FolderConfigSettings.setInstance(), add null guards on explicit-change methods - Extract 21 LS setting key constants to LsSettingsKeys - Add 21 new tests covering change tracking, payload construction, and round-trip semantics
… getCurrentSettings [IDE-1652] Migrate HTMLSettingsPreferencePage, NativeProjectPropertyPage, ReferenceChooserDialog, and SnykToolView from old FolderConfigs/FolderConfig to FolderConfigSettings/LspFolderConfig. Add bridge in folderConfig() to populate both old and new config systems during transition. Remove unused getCurrentSettings() from LsConfigurationUpdater and its test.
…v25 [IDE-1652] Delete FolderConfig, FolderConfigsParam, FolderConfigs, and Settings classes. Remove old $/snyk.folderConfigs handler and SNYK_FOLDER_CONFIG constant. Bump REQUIRED_LS_PROTOCOL_VERSION from 23 to 25. Resolve review findings: token changed flag uses isExplicitlyChanged, atomic computeFolderConfig for consumer writes, additionalParameters returns List<String>, folderPath set on empty configs.
…aths [IDE-1652] Replace prefs.store() with prefs.storeAndTrackChange() in all user-facing settings pages (HTMLSettingsPreferencePage, PreferencesPage, BaseHandler, SnykWizard, SummaryBrowserHandler) so that user edits are tracked as explicitly changed. Wire folder-scope consumers to use withSettingIfChanged instead of withSetting. Update LsConfigurationUpdater to propagate isExplicitlyChanged for all user-changeable settings in the outbound payload. Remove unused PREF_TO_LS_KEY field.
…user overrides [IDE-1652] storeAndTrackChange compared against hardcoded empty string default instead of the registered store default, causing settings with non-empty defaults to be falsely marked as user-overridden on first save. HTML settings form now handles null values from LS as reset signals, calling clearExplicitlyChanged to remove the user override before the next didChangeConfiguration is sent to the LS.
Use Map interface instead of ConcurrentHashMap for field declaration. Narrow catch clause from Exception to IllegalArgumentException.
…ettings [IDE-1652] Replaces the hardcoded inline outbound block in LsConfigurationUpdater and the 8-key LS_TO_PREF_KEY map in SnykExtendedLanguageClient with a shared LsSettingsRegistry. Both outbound (IDE→LS) and inbound ($/snyk.configuration) now use the same binding. Also adds snyk_secrets_enabled (outbound + inbound) which was previously missing.
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.
…ing [IDE-1652] addAll() previously called configs.clear() before re-adding, which wiped user overrides (changed=true) in flight. Now merges per-folder: incoming wins for unchanged keys, user-changed keys survive; folders absent from incoming payload are removed.
…urationParam [IDE-1652] Drops snake_case @SerializedName annotations from ConfigSetting, LspFolderConfig, and LspConfigurationParam — LS now uses camelCase JSON keys matching Java field names. Adds metadata fields (requiredProtocolVersion, integrationName, trustedFolders, etc.) to LspConfigurationParam for InitializationOptions serialization.
Four bugs found by cross-checking with snyk-ls internal/types/ldx_sync_config.go: 1. scan_automatic: was sending string "automatic"/"manual"; LS GetBool() only returns true for string "true" → auto-scan always false. Now sends Java Boolean true/false. 2. enabled_severities: LS has no such key. Uses individual severity_filter_critical/high/ medium/low boolean keys. Replace composite map with 4 individual settings. 3. enableTelemetry: no such key in LS. Was sending camelCase "enableTelemetry" which LS silently ignored. Remove from outbound registry entirely. 4. additional_environment: LS registers this as folder-scope only, not global. Remove from global settings outbound; stays in folderConfigs[].settings path.
…[IDE-1652] Pull latest from snyk-ls@270c5ad4 (feat(config): centralized configuration [IDE-1786]). Adds custom CLI channel UI, updated CSS variables, and other config dialog changes.
UseVarargs, ArrayIsStoredDirectly, MethodReturnsInternalArray on trustedFolders. Use varargs setter, clone on store and on return.
Moves verifyCliProtocolVersion() and its tests to feat/IDE-1652-cli-protocol-version-gate branch.
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
This comment has been minimized.
This comment has been minimized.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| ProcessBuilder pb = new ProcessBuilder(cliPath, "language-server", "--protocolVersion"); | ||
| pb.redirectErrorStream(true); | ||
| Process proc = pb.start(); | ||
| String output = new String(proc.getInputStream().readAllBytes()).trim(); |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…an triggers [IDE-1652] Replace the flat LsSettingsKeys string constants with a typed LsKey enum so all LS protocol key references are compiler-checked and co-located with their string value. LsSettingsRegistry ENTRIES is now an EnumMap<LsKey, Entry> with insertion order preserved; BY_LS_KEY retains the string-keyed lookup for inbound parsing. Entry gains alwaysChanged (replaces the outbound-side of alwaysFixed) and useInFallbackForm flags. TOKEN is now alwaysChanged=true so it is always sent with changed=true without requiring explicit-change tracking. Inbound deserializers for risk_score_threshold and trusted_folders are added to BY_LS_KEY so persistGlobalSettings can handle them directly; the alwaysFixed guard is removed from that method since prefKey==null already excludes hardcoded entries. HTMLSettingsPreferencePage.parseAndSaveConfig is rewritten as a registry-driven JsonNode loop, removing the IdeConfigData typed record and its fragile field-by- field mapping. FolderConfigSettings.createEmptyConfig switches to direct setters instead of round-tripping through Gson. LspFolderConfig gains setFolderPath and setSettings mutators to support this. snykConfiguration() restores the two behaviours dropped when folderConfig() was removed: triggerScan(null) when SCANNING_MODE_AUTOMATIC is set, and toolView.refreshDeltaReference() after folder configs are stored. These are needed for LDX-sync post-startup pushes where the LS does not trigger a scan. All LsSettingsKeys references in LsConfigurationUpdater, LsConfigurationUpdaterTest, and SnykLanguageServerTest are updated to LsKey.FOO.key.
…pattern [IDE-1652] Move RISK_SCORE_THRESHOLD, SEVERITY_FILTER_*, and TRUSTED_FOLDERS from inline blocks in LsConfigurationUpdater into LsSettingsRegistry.ENTRIES so all three data paths (IDE→LS, LS→IDE, form→IDE) are driven by pure registry loops. - LsSettingsRegistry: add additionalChangedPrefKeys (severity shared-changed flag) and formDeserializer (form-specific JSON→pref conversion) to Entry; move all previously inline-only keys into ENTRIES; BY_LS_KEY now auto-populated from ENTRIES - LsConfigurationUpdater: remove 3 inline blocks; single generic loop with additionalChangedPrefKeys OR-ing for changed flag - HTMLSettingsPreferencePage: remove FALLBACK_FORM_KEYS set, SCANNING_MODE special-case, and post-loop severity/risk/trusted blocks; pure registry loop using entry.useInFallbackForm and entry.formDeserializer - FolderConfigSettings.addAll: remove mergeConfigs — LS values are authoritative, incoming folder config replaces existing entirely (matches VSCode behaviour)
Replace per-key if-blocks with a generic folderNode.fields() loop. scan_command_config stays special-cased for typed deserialization; all other fields use withSetting(key, value, true) directly, matching VSCode's setSetting pattern. Removes convertScanCommandConfig helper.
- HTMLSettingsPreferencePage: replace manual ScanCommandConfig construction loop with objectMapper.convertValue() — fixes AvoidInstantiatingObjectsInLoops - LsSettingsRegistry: suppress AvoidFieldNameMatchingMethodName on alwaysChanged field; extract TRUE constant for severity filter outbound defaults — fixes AvoidDuplicateLiterals
Resolves PMD AvoidFieldNameMatchingMethodName — field clashed with the Entry.alwaysChanged() factory method. Rename field; factory method and constructor parameter keep the same name.
- HTMLSettingsPreferencePageTest: update JSON keys from camelCase to snake_case LS key strings (cli_path, automatic_download, snyk_code_enabled etc); flatten filterSeverity/issueViewOptions nested objects to individual severity_filter_* and issue_view_* keys — matching what the LS HTML sends - FolderConfigSettingsTest: remove addAllPreservesUserOverriddenKeys — merge behavior was intentionally removed; LS values are authoritative - LsConfigurationUpdaterTest: add getPref stubs for severity filters and scanning_mode — registry uses getPref not getBooleanPref
…[IDE-1652] Replace hardcoded pref key array with a stream over ENTRIES — any new registry key is automatically tracked without a separate update here.
Two fixes to ensure user-set values survive restart: 1. parseAndSaveConfig: unconditionally mark all non-null form values as explicitly changed. Old equality check could skip marking when persistGlobalSettings had already written the same value, leaving explicitChanges empty on disk. 2. buildConfigurationParam: treat stored value deviating from outbound default as changed=true, matching IntelliJ's machineScopedValueDeviatesFromPluginDefaults logic. Covers values set via login flow (saveLoginArgs) which bypass explicit change tracking.
processFolderConfig fell through to node.asText() for array nodes, producing an empty string for additional_parameters (sent as JSON array by LS HTML). Add array branch that collects elements into List<String>, matching IntelliJ's StringOrListTypeAdapter behaviour.
parseAndSaveConfig treated absent keys (n == null) and explicit null (n.isNull()) identically — both called clearExplicitlyChanged. Since the LS HTML sends only diff (user-changed keys), every save wiped tracking for all untouched keys, leaving explicitChanges empty. Split the two cases: absent = leave tracking untouched; explicit null = clear. Matches IntelliJ SaveConfigHandler behaviour.
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
force-pushed
the
feat/IDE-1652-cli-protocol-version-gate
branch
from
a650f99 to
0936ad9
Compare
This comment has been minimized.
This comment has been minimized.
PR Reviewer Guide 🔍
|
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
3 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.
Description
Adds
verifyCliProtocolVersion()called fromSnykLanguageServer.start()after the CLI path is validated. If the CLI reports a protocol version that doesn't matchREQUIRED_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.