Skip to content

feat(language-server): bidirectional LSP configuration protocol [IDE-1638]#733

Open
acke wants to merge 42 commits intomainfrom
feat/IDE-1638-snyk-configuration-listener
Open

feat(language-server): bidirectional LSP configuration protocol [IDE-1638]#733
acke wants to merge 42 commits intomainfrom
feat/IDE-1638-snyk-configuration-listener

Conversation

@acke
Copy link
Copy Markdown
Contributor

@acke acke commented Mar 20, 2026

Summary

Implements the full bidirectional LSP configuration protocol between the VS Code extension and Snyk Language Server (protocol v25+), replacing the legacy flat-settings push with structured LspConfigurationParam payloads.

  • Inbound ($/snyk.configuration): register notification handler, merge global + per-folder settings into a unified view, apply isLocked/source/originScope hints to the workspace configuration webview, and persist the LS snapshot to VS Code settings
  • Outbound (pull model): middleware intercepts workspace/configuration requests and responds with structured LspConfigurationParam (including changed flags via ExplicitLspConfigurationChangeTracker so LS distinguishes user overrides from echoed values)
  • Folder configs: synthesize per-folder rows from workspace folders when LS hasn't sent any; replace the FolderConfig plain type with a class wrapping LSP settings; remove $/snyk.folderConfigs legacy notification
  • Auth: accept expired OAuth tokens when a refresh token is present; fix token validation to use expiry field
  • Misc: upgrade lodash to 4.18.1 (CVE fix), sync settings-fallback.html with upstream snyk-ls, centralize LS-to-VSCode key mapping

Key modules added/changed

Module Role
serverSettingsToLspConfigurationParam.ts Converts flat VS Code settings → structured LspConfigurationParam for outbound
lsKeyToVscodeKeyMap.ts Centralized bidirectional LS ↔ VS Code key mapping
explicitLsKeyTracking.ts / explicitLspConfigurationChangeTracker.ts Tracks which settings the user explicitly changed vs LS-originated
inboundLspConfigurationToHtmlSettings.ts Maps inbound LS config → webview HTML settings
inboundLspFolderSettingsToFolderConfig.ts Maps inbound LS folder settings → FolderConfig instances
languageServer.ts Registers $/snyk.configuration, merges inbound config, wires persistence
middleware.ts Intercepts workspace/configuration pull requests, responds with structured payload
configurationWatcher.ts Simplified — delegates to explicit key tracker
configurationPersistenceService.ts Persists inbound LS config snapshot to VS Code settings

Test plan

  • Unit tests for serverSettingsToLspConfigurationParam (round-trip mapping, changed flags, folder configs, reset-to-default)
  • Unit tests for inboundLspConfigurationToHtmlSettings and inboundLspFolderSettingsToFolderConfig
  • Unit tests for language server notification registration and inbound config handling
  • Unit tests for middleware structured response with explicit change tracking
  • Unit tests for ConfigurationPersistenceService inbound persistence
  • Unit tests for AuthenticationService token validation with expiry and refresh tokens
  • Integration test cleanup (removed tests for deleted functionality)

Checklist

@snyk-io
Copy link
Copy Markdown

snyk-io bot commented Mar 20, 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.

@acke acke force-pushed the feat/IDE-1638-snyk-configuration-listener branch 5 times, most recently from a242438 to 11e5657 Compare March 31, 2026 10:29
Comment thread src/snyk/common/views/workspaceConfiguration/services/htmlInjectionService.ts Outdated
@acke acke force-pushed the feat/IDE-1638-snyk-configuration-listener branch from 11e5657 to 3cd2944 Compare March 31, 2026 12:56
acke added 15 commits March 31, 2026 15:05
…-1638]

- Add SNYK_CONFIGURATION constant and LspConfigurationParam types
- Handle notification with stub logging until UI/state wiring
- Unit test: handler registered and invoked
- Gitignore local *_implementation_plan/ directories

Made-with: Cursor
- Add mergeInboundLspConfiguration and expose getInboundLspConfigurationView
- Unit tests use snyk-ls PR #1162 map keys (endpoint, activateSnykCode, etc.)
- Document GAF → snyk-ls → IDE flow and merge points in docs/

Made-with: Cursor
…ig to webview [IDE-1638]

- Register LSP notification, merge inbound payload with folder configs, forward to workspace config provider
- Extend workspace configuration types and provider with inbound LSP config callback
- Refactor language server unit tests with shared recording client adapter helpers; drop brittle sinon.verify in startup tests

Tests: npm run lint:fix, npm run test:unit
Snyk Code: 1 pre-existing low finding in treeViewWebviewScript.ts (unchanged)

Made-with: Cursor
…ebview [IDE-1638]

- Post merged $/snyk.configuration view to webview; cache and replay on panel open
- Inject DOM helper: data-snyk-setting-key / data-snyk-folder-path, isLocked, source/originScope
- Shared INBOUND_LSP_CONFIGURATION_MESSAGE constant; integration + unit tests; docs

Tests: npm run lint:fix, npm run test:unit
Snyk Code: pre-existing low finding in treeViewWebviewScript.ts (unchanged)

Made-with: Cursor
…S [IDE-1638]

- Replace synchronize.configurationSection with explicit structured pushes (pflag maps)
- Debounce on snyk/http configuration changes and mark explicit pflags from VS Code and IDE config
- Add serverSettingsToLspConfigurationParam, explicit tracker, and ideConfig explicit pflags
- Extend IVSCodeWorkspace with onDidChangeConfiguration for the listener
- Document the flow in docs/configuration-gaf-ls-ide-flow.md

Made-with: Cursor
…idation [IDE-1638]

- Sync snyk:loggedIn from stored credentials after LS start and on $/snyk.configuration / token changes
- Relax OAuth validateToken when access expiry is stale but refresh_token is present (welcome view)
- Document getToken semantics; warn on secret storage read failure
- Align welcome view container when-clause with snyk:authMethodChanged context key
- Expose syncLoggedInContextFromStoredTokenIfValid on IExtension for watcher and tests

Made-with: Cursor
On the first non-empty merged global payload from $/snyk.configuration, map
pflags to IdeConfigData and write through the same path as the workspace
configuration UI (scope rules and skip unchanged). Suppress outbound
structured didChangeConfiguration while applying to avoid echo. Token
updates go to secret storage only.

- Add inboundLspConfigurationToIdeConfig and unit tests
- Extend ILanguageServer with setInboundConfigurationPersistenceHandler

Made-with: Cursor
…[IDE-1638]

Remove one-shot startup flag; run persistInboundLspConfiguration for each
notification with mappable globals. Serialize writes via inboundPersistenceChain
and recover the chain on rejection. Rename API from persistInboundLspConfigurationOnStartup;
handler returns void.

Made-with: Cursor
…E-1638]

Add filterInboundPartialByExplicitOverrides so persistInboundLspConfiguration
does not write conflicting values from $/snyk.configuration when the user has
explicitly overridden the corresponding pflags (memento-backed tracker).

Wire reconcileLanguageServerWithCurrentConfiguration from LanguageServer into
ConfigurationPersistenceService for reconcile after filtered persistence.
Extend unit tests and configuration flow docs.

Made-with: Cursor
…-1638]

- Define LspInitializationOptions aligned with snyk-ls (settings pflag map, folderConfigs, init metadata).

- Build initializationOptions via serverSettingsToLspInitializationOptions from flat ServerSettings.

- Move received-folder-configs flag to receivedFolderConfigsFromLsState to avoid circular imports.

- Extend LanguageServerSettings.fromConfiguration and update unit tests.

Made-with: Cursor
…DE [IDE-1638]

- Emit organization when set, including empty string; omit only when unset
- Omit endpoint, CLI path, binary base URL, and token when empty or whitespace-only
- Emit risk_score_threshold only when value is not null or undefined (0 is still sent)
- Add and adjust unit tests

Made-with: Cursor
…s [IDE-1638]

When in-memory folder configs are empty (LS has not sent $/snyk.folderConfigs yet), build minimal FolderConfig rows from workspace.getWorkspaceFolders() using the same org rules as mergeOrgSettingsIntoLSFolderConfig.

LanguageServerSettings.fromConfiguration accepts optional workspace; LanguageServer and middleware pass IVSCodeWorkspace.

Document in CHANGELOG Unreleased.

Made-with: Cursor
…gement [IDE-1638]

Changing snyk.advanced.cliReleaseChannel no longer stops Snyk LS or triggers CLI download when automatic dependency management is disabled, since the extension does not manage the binary in that mode.

Document in CHANGELOG Unreleased.

Made-with: Cursor
- Apply per-folder pflags from $/snyk.configuration to in-memory FolderConfig via mergeFolderConfigsWithInboundLspView and setFolderConfigs (no outbound didChangeConfiguration echo).
- Extend inbound persistence / merge tests for folder-scoped settings (e.g. scan_net_new) and add unit tests for the new merge helper.

Related LSP configuration listener and persistence updates on this branch are included in this commit.

Made-with: Cursor
…IDE-1638]

- resolveFolderConfigsForServerSettings uses getFolderConfigs() when non-empty, else synthesize from workspace folders
- Remove ReceivedFolderConfigsFromLs, receivedFolderConfigsFromLsState, and SNYK_FOLDERCONFIG registration
- Simplify fromConfiguration(configuration, user, workspace?) call sites
- Drop obsolete handleOrgSettingsFromFolderConfigs tests; refresh init options tests
- Update CHANGELOG Unreleased

Made-with: Cursor
@acke acke force-pushed the feat/IDE-1638-snyk-configuration-listener branch from 3cd2944 to 5eec5ce Compare March 31, 2026 13:38
acke and others added 2 commits March 31, 2026 15:38
…er [IDE-1638]

Removed layers that contradicted "LS is the source of truth":

- Removed inbound merge layer (lspConfigurationMerge.ts): the IDE was
  re-merging global settings into folder settings, but the LS already
  sends fully-resolved effective configs per folder.

- Removed inbound explicit override filter (inboundLspExplicitOverrideFilter.ts):
  the IDE was blocking LS values that conflicted with user-set IDE
  settings. The correct flow is: user changes a setting → IDE sends
  didChangeConfiguration with changed: true → LS resolves precedence
  → LS sends back effective result via $/snyk.configuration → IDE
  trusts it 100%.

- Removed reconcile mechanism (reconcileLanguageServerWithCurrentConfiguration):
  existed to push IDE values back to LS when the filter detected
  conflicts. No longer needed since inbound config is trusted.

- Removed IDE-side webview DOM manipulation for lock enforcement and
  source/origin labels (htmlInjectionService): the LS sends the
  settings HTML page with locks and metadata already rendered. The IDE
  was duplicating this by injecting JS that matched data-snyk-setting-key
  attributes and toggled disabled/locked states.

- Removed inbound config push to webview (onInboundLspConfigurationUpdated,
  scheduleInboundConfigurationNotificationToWorkspace, INBOUND_LSP_CONFIGURATION_MESSAGE):
  no longer needed since the LS HTML handles its own UI state.

- Replaced folder config merge with direct replacement
  (folderConfigsFromLspParam): the IDE was merging inbound folder
  configs with existing in-memory state. Since LS is source of truth,
  inbound folder configs now replace the in-memory list entirely.

Additional changes:

- Bumped PROTOCOL_VERSION from 24 to 25.
- Added try-catch to handleSnykConfigurationNotification for exception
  safety (NFR8).
- Added reset-to-default support: when a string setting is null/undefined
  and explicitly changed, sends { value: null, changed: true } to tell
  the LS to revert to the resolved default.
- Renamed PFLAG → LS_KEY, pflagKey → lsKey for clarity.
- Wired folderConfigsFromLspParam into persistInboundLspConfiguration
  so inbound folder configs are applied to in-memory storage.
@nick-y-snyk nick-y-snyk force-pushed the feat/IDE-1638-snyk-configuration-listener branch from e74357d to 6cf2559 Compare April 2, 2026 13:34
…g LSP settings [IDE-1638]

FolderConfig is now a class backed by Record<string, LspConfigSetting>,
eliminating the LS↔IDE conversion layer. Typed getter/setter methods
provide ergonomic access to the ~6 fields VS Code uses; all other LS
settings pass through transparently without explicit mapping.
…odel [IDE-1638]

Replace explicit didChangeConfiguration push with synchronize.configurationSection
pull model. The middleware now returns structured LspConfigurationParam responses
with proper `changed` flags via ExplicitLspConfigurationChangeTracker, instead of
the language server pushing notifications directly.
The expiry check in validateToken rejected OAuth tokens with an expired
access_token even when a valid refresh_token was available. The LS can
use the refresh token to obtain a new access token, so these should be
accepted.
Comment thread src/snyk/common/languageServer/serverSettingsToLspConfigurationParam.ts Outdated
Comment thread src/snyk/common/languageServer/serverSettingsToLspConfigurationParam.ts Outdated
Comment on lines +193 to +195
if (settings.hoverVerbosity !== undefined) {
putSetting(m, LS_KEY.hoverVerbosity, settings.hoverVerbosity, isExplicitlyChanged);
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is only LS->IDE

Comment on lines +197 to +199
if (settings.trustedFolders !== undefined) {
putSetting(m, LS_KEY.trustedFolders, settings.trustedFolders, isExplicitlyChanged);
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doesn't need special handling

Comment on lines +201 to +211
if (
settings.secureAtInceptionExecutionFrequency !== undefined &&
settings.secureAtInceptionExecutionFrequency !== ''
) {
putSetting(
m,
LS_KEY.secureAtInceptionExecutionFreq,
settings.secureAtInceptionExecutionFrequency,
isExplicitlyChanged,
);
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't need special handling

…ix stale doc references

- Remove syncLoggedInContextFromStoredTokenIfValid: $/snyk.hasAuthenticated
  already sets LOGGEDIN context on auth; the redundant sync on every
  $/snyk.configuration notification, startup, and token change was
  unnecessary (not present on main)
- Fix docs/configuration-gaf-ls-ide-flow.md: replace 5 nonexistent symbol
  references with actual code identifiers
- Fix phantom HtmlSettingsLsFields JSDoc link in LS_KEY
- Use direct VS Code setting constants in configurationMappingService
  instead of LS_KEY_TO_VSCODE_KEY lookups
@snyk-pr-review-bot

This comment has been minimized.

…ess PR review comments

Remove the ServerSettings intermediate type so fromConfiguration() returns
LspConfigurationParam directly, collapsing the double-mapping chain
(IConfiguration → ServerSettings → LspConfigurationParam) into a single step.

PR review fixes: remove AUTHENTICATING from initiateLogout, rename
inboundPersistenceChain → configPersistenceQueue, drop duplicate http config
tracking, remove LS_KEY_ALWAYS_CHANGED_* overrides, remove hoverVerbosity
from outbound config (LS→IDE only), remove special handling for trustedFolders
and secureAtInceptionExecutionFrequency.

Additional simplifications: data-driven mapLspSettingsToHtmlSettings loop,
VALUE_TRANSFORMATIONS map in configurationMappingService, feature toggle loop
in fromConfiguration, exported VSCODE_KEY_TO_LS_KEYS reverse index, removed
User param from middleware constructor.
@snyk-pr-review-bot

This comment has been minimized.

? 'true'
: `${featuresConfiguration?.codeSecurityEnabled}`;
putSetting(m, LS_KEY.hoverVerbosity, 1, isExplicitlyChanged);
putSetting(m, LS_KEY.trustedFolders, configuration.getTrustedFolders(), isExplicitlyChanged);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

settings that had explicit value set, should still use an explicit value.
e.g. TrustedFolders was always sent as true

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also automaticAuthentication was false

 with declarative SETTINGS_REGISTRY
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Introduce a single declarative registry that is the source of truth for
LS key → VS Code key → IConfiguration resolver mapping, with compile-time
enforcement via Record<GlobalLsKeyValue, RegistryEntry>.

- Split LS_KEY into LS_GLOBAL_KEY (27 settings) + LS_FOLDER_KEY (9 settings);
  adding a new global key without a registry entry now causes a compile error
- Replace ~90-line procedural fromConfiguration() with a ~15-line generic loop
  where every non-alwaysChanged setting is resettable by default
- Delete ConfigurationMappingService class and inboundLspConfigurationToHtmlSettings;
  replace with standalone mapConfigToSettings(), mapLspSettingsToVscodeSettings(),
  and mapHtmlKeyToVSCodeSetting() functions driven by the registry
- Remove ConfigurationMappingService DI from persistence service, scope detection
  service, webview provider, and extension wiring
@snyk-pr-review-bot

This comment has been minimized.

…ound LS persistence

When $/snyk.configuration persists settings to VS Code settings.json,
the resulting onDidChangeConfiguration event was bouncing a
workspace/didChangeConfiguration notification back to the LS. Reuse the
existing suppress flag (renamed to suppressConfigFeedbackFromInboundPersistence)
in a new didChangeConfiguration middleware hook to break the cycle.
@snyk-pr-review-bot

This comment has been minimized.

@snyk-pr-review-bot

This comment has been minimized.

…nfiguration-listener

# Conflicts:
#	media/views/common/configuration/settings-fallback.html
#	package-lock.json
@snyk-pr-review-bot

This comment has been minimized.

@snyk-pr-review-bot

This comment has been minimized.

…901]

- Change LS_GLOBAL_KEY.trustedFolders from 'trustedFolders' to
  'trusted_folders' to match the pflag setting name on the LS side.
- Mark as alwaysChanged since it's not part of LDX sync.
- Remove top-level trustedFolders from LspInitializationOptions and
  init options builder — LS now reads it from the settings map.
@snyk-pr-review-bot

This comment has been minimized.

@snyk-pr-review-bot

This comment has been minimized.

* fix: update severity filter keys and adjust related settings logic

* fix: integrate context service into configuration persistence logic

- Added context service to the SnykExtension for improved state management.
- Updated ConfigurationPersistenceService to utilize context service for setting authentication-related contexts when saving configuration.
- Enhanced unit tests for FolderConfig to ensure correct behavior of setting changes.
@snyk-pr-review-bot

This comment has been minimized.

…ttings

- Added unit test assertions to verify the values of severity filter settings (critical, high, medium, low) in LanguageServerSettings.
- Ensured that the settings reflect the expected configuration values from the mock configuration.

This change enhances the test coverage for the LanguageServerSettings component.
@snyk-pr-review-bot
Copy link
Copy Markdown

PR Reviewer Guide 🔍

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

Breaking Interface Change 🟠 [major]

The transition of FolderConfig from a type to a class with method accessors (e.g., baseBranch() instead of baseBranch) is a breaking change for any un-migrated downstream consumers. While most internal callers were updated, any code outside this PR that still treats FolderConfig as a plain data object will throw runtime errors on property access.

export class FolderConfig {
  readonly folderPath: string;
  settings: Record<string, LspConfigSetting>;
Feedback Suppression Stalling 🟡 [minor]

The suppressConfigFeedbackFromInboundPersistence flag is reset in a finally block within the configPersistenceQueue promise chain. If the persistInboundConfiguration handler hangs indefinitely (e.g., due to a stalled VS Code configuration update), the flag will stay true permanently, preventing all subsequent extension-side configuration changes from propagating back to the Language Server.

    this.suppressConfigFeedbackFromInboundPersistence = false;
  }
});
📚 Repository Context Analyzed

This review considered 146 relevant code sections from 13 files (average relevance: 0.87)

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