Skip to content

refactor(oss): delegate C/C++ auto-detect to CLI extension [IDE-2089]#1311

Open
acke wants to merge 8 commits into
mainfrom
feat/IDE-2089_unmanaged-cpp-scan
Open

refactor(oss): delegate C/C++ auto-detect to CLI extension [IDE-2089]#1311
acke wants to merge 8 commits into
mainfrom
feat/IDE-2089_unmanaged-cpp-scan

Conversation

@acke

@acke acke commented May 30, 2026

Copy link
Copy Markdown
Contributor

Summary

This PR was originally cut to ship a snyk-ls–side C/C++ auto-detect prompt + per-folder unmanaged toggle. After review, the detection + decision was moved into the CLI's cli-extension-os-flows instead. snyk-ls now only sets one env var; everything else is the extension's responsibility.

Net diff against main: 4 files, +62 / -0.

The intermediate work (detector, prompt, per-folder setting, command, panel toggle, re-arm logic, etc.) was added and then removed in subsequent commits on this branch, so it does not appear in the cumulative file diff. See commit history for the steps.

What snyk-ls actually changes here

  • infrastructure/oss/cli_scanner.go — sets SNYK_AUTODETECT_OSS=1 in the env passed to every OSS CLI invocation. The CLI's os-flows extension then decides per input directory whether to also run an unmanaged scan.
  • infrastructure/oss/cli_scanner_test.go — new test asserting the env-var contract (TestCLIScanner_prepareScanCommand_SetsAutodetectEnv).
  • docs/requirements/architecture.md — ADR describing the new delegation, including the limitation (see below).
  • docs/requirements/requirements.md — 3 updated IDE-2089 requirements reflecting the new boundary.

How the auto-detect now works (CLI side, see cli-extension-os-flows#251)

snyk test … (with SNYK_AUTODETECT_OSS=1 from snyk-ls)
   │
   └─► OSWorkflow
         ├─► managed scan (existing path)
         └─► detect.HasCPPArtefacts(inputDir) for each input dir
                  │
                  └─► any C/C++ artefacts?
                          ├─ no  → return managed data
                          └─ yes → invoke legacycli --unmanaged
                                   append its workflow.Data to managed output

The extension keeps the previous --unmanaged flag's semantics for direct CLI users; the env var is the only new switch.

Limitation

A native Go unmanaged.test workflow does not yet exist; the extension currently invokes the legacy TypeScript CLI to perform the unmanaged scan when C/C++ is detected. The legacy CLI's workflow.Data carries its own content type, so for mixed manifest + C/C++ folders the unmanaged and managed results render as two separate sections rather than as one unified report. When the native workflow lands, the extension swap is local — no snyk-ls change needed.

Why this shape

  • No IDE prompt and no per-folder toggle. Detection happens unconditionally on every OSS scan; the user never has to opt in. The C/C++ walker is bounded (max 5000 entries, depth 6, skipping node_modules/vendor/cmake-build-*/VCS dirs) and short-circuits on the first hit, so the cost is minimal.
  • One source of truth. Detection rules now live with the rest of the OSS routing in cli-extension-os-flows/pkg/unmanaged/detect. Future plugin work (VS Code, Eclipse, Visual Studio) gets the same behaviour automatically since they all invoke the same CLI.
  • Backwards-compatible for non-IDE CLI users. The auto-detect is off unless SNYK_AUTODETECT_OSS=1 is set; direct snyk test invocations behave exactly as before.

Depends on / related

  • cli-extension-os-flows#251 — adds the detector, OSWorkflow orchestration, and the env-var gate. Must merge before this snyk-ls PR has any user-visible effect.
  • snyk-intellij-plugin#834 — closed; the per-folder snyk_oss_unmanaged_enabled setting it forwarded no longer exists.

Jira: IDE-2089

Test plan

  • go build ./... clean
  • go vet ./... clean
  • go test ./internal/types/... ./infrastructure/oss/... ./infrastructure/configuration/... — green
  • make test-js — 98/98 pass against regenerated config-page.html fixture
  • make build-debug in /cli against this branch — clean (binary built, LS commit pinned)
  • Manual end-to-end: requires cli-extension-os-flows#251 merged + CLI dep bumped. Open a folder with both package.json and .cpp files in the IDE and confirm both managed and unmanaged scan results appear.

🤖 Generated with Claude Code

…d scan [IDE-2089]

When an OSS scan runs on a folder containing C/C++ source, header, or
build-system files, prompt the user once to enable unmanaged scanning.
The user's choice is persisted per folder; subsequent scans append
--unmanaged to the CLI invocation automatically.

- HasCPPArtefacts: bounded WalkDir (max 5000 entries, depth 6) that
  short-circuits on the first C/C++ extension or build artefact and
  skips node_modules, vendor, cmake-build-*, VCS, and common build dirs.
- maybePromptForUnmanagedScan: fired from CLIScanner.Scan once per
  folder; persists snyk_oss_unmanaged_prompted before sending so a
  crash mid-prompt cannot re-prompt.
- EnableUnmanagedScanCommand: handler for the prompt's Yes action;
  sets snyk_oss_unmanaged_enabled=true for the folder.
- Both settings are folder-scoped, local-only (not LDX-synced).
- Adds requirements + architecture doc for IDE-2089.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@snyk-io

snyk-io Bot commented May 30, 2026

Copy link
Copy Markdown

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.

… Open Source [IDE-2089]

The auto-detect prompt is the primary enablement path, but users who
dismissed it (or want unmanaged scanning on folders without C/C++ files)
had no UI to change snyk_oss_unmanaged_enabled. Adds an indented
sub-toggle directly under the Snyk Open Source row in the per-folder
Scanners section.

- config.html: new checkbox bound to folder_INDEX_snyk_oss_unmanaged_enabled.
- styles.css: .checkbox-sub for the visual indent.
- form-handler.js + reset-handler.js: include the new field in folder reset.
- form-payload snapshot regenerated.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…089]

The IDE-2089 architecture section covered only the prompt-driven
enablement path. Adds a paragraph documenting the per-folder UI toggle
in the Snyk Workspace Configuration panel and the three cases it
covers that the prompt cannot (no-C/C++ folders, post-dismissal,
discoverability). Also notes that the panel is served from snyk-ls,
not the IDE plugin.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@snyk-pr-review-bot

This comment has been minimized.

@snyk-pr-review-bot

This comment has been minimized.

…fig page [IDE-2089]

Three fixes after testing in the IDE:

1. The config-page checkbox always rendered as unchecked even when the
   user-folder setting was true. computeEffectiveConfig() in
   configuration_command.go has a hardcoded allowlist of settings to
   expose to the HTML template; SettingSnykOssUnmanagedEnabled was
   missing from it. The Yes-action persisted correctly, but the panel
   couldn't read it back.

2. Both the prompt message and the checkbox label said "also scan
   unmanaged". --unmanaged actually REPLACES manifest-based OSS
   scanning with file-hash matching, so "also" was misleading. Rewrote
   to make clear it's a mode switch — if a folder has both C/C++ files
   and a supported manifest, enabling unmanaged mode stops the OSS
   scan from looking at the manifest.

3. The .checkbox-sub indent was too subtle to read as a child of the
   parent toggle. Adds a left border, larger padding, slight
   transparency so the parent-child relationship is unmistakable.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@snyk-pr-review-bot

This comment has been minimized.

…disable [IDE-2089]

Two related UX improvements based on testing in VS Code:

1. Hide the per-folder unmanaged-mode sub-toggle when the folder does
   not contain any C/C++ files. Adds an in-process memoizing cache
   (HasCPPArtefactsCached) so the bounded WalkDir runs at most once
   per folder per LS process — shared between the auto-detect prompt
   and the settings panel. Template helper hasUnmanagedArtefacts gates
   the checkbox via {{if ...}}.

2. Re-arm the auto-detect prompt when the user explicitly disables
   unmanaged mode via the panel. ApplyLspUpdate now clears the
   snyk_oss_unmanaged_prompted latch when enabled goes to false (or is
   reset to default). This distinguishes "I dismissed/answered the
   prompt" (latch stays sticky to avoid pestering) from "I actively
   turned this off via settings" (user explicitly disengaged — they
   should see the prompt again if they later move into a C/C++
   project context).

The snapshot diff is a fixture-regen consequence; no protocol changes.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@snyk-pr-review-bot

This comment has been minimized.

…089]

The unmanaged checkbox sits in a div that carries both .checkbox-sub
and .override-indicator-wrapper. Both were single-class selectors, and
.override-indicator-wrapper (margin-left: -8px) was defined later in
the stylesheet so it won by source order — the 24px indent from
.checkbox-sub never reached the DOM, and the row visually aligned with
the other top-level scanner checkboxes.

Splits .checkbox-sub into the typography rule plus a compound
selector .checkbox-sub.override-indicator-wrapper that beats the bare
override wrapper on specificity. The sub-toggle now indents 28px under
its parent with a clear vertical guide border.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@snyk-pr-review-bot

This comment has been minimized.

…extension [IDE-2089]

Detection and the "should this folder run unmanaged?" decision now live
in cli-extension-os-flows; snyk-ls just sets SNYK_AUTODETECT_OSS=1 on
every OSS CLI invocation and lets the extension orchestrate the rest.

Removes — all obsolete with the decision moved into the extension:

  - infrastructure/oss/unmanaged_detect.go + test  (file-system walker)
  - infrastructure/oss/unmanaged_prompt.go + test  (window/showMessageRequest UX)
  - domain/ide/command/enable_unmanaged_scan.go + test  (Yes-action handler)
  - SettingSnykOssUnmanagedEnabled / SettingSnykOssUnmanagedPrompted
    pflag registrations, ldx_sync_config constants, and config_resolver
    re-arm-prompt logic + tests
  - EnableUnmanagedScanCommand constant and command_factory case
  - hasUnmanagedArtefacts template helper and the `{{if ...}}` block
    in config.html
  - .checkbox-sub CSS rule
  - snyk_oss_unmanaged_enabled entries from form-handler.js reset path
    and reset-handler.js section defaults
  - --unmanaged injection in cli_scanner.prepareScanCommand and the
    maybePromptForUnmanagedScan call in CLIScanner.Scan
  - SettingSnykOssUnmanagedEnabled from computeEffectiveConfig's
    org-scope settings allowlist

Adds:

  - SNYK_AUTODETECT_OSS=1 to the env passed to the OSS CLI in
    cli_scanner.prepareScanCommand
  - TestCLIScanner_prepareScanCommand_SetsAutodetectEnv covers the
    new env-var contract

Updates architecture.md / requirements.md to describe the new flow and
to call out the current limitation: the extension invokes the legacy
TypeScript CLI for the unmanaged scan (no native unmanaged.test
workflow yet), so mixed manifest+C/C++ folders render results as two
separate sections rather than as one unified report.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@snyk-pr-review-bot

This comment has been minimized.

Result of `git diff origin/main` after refactor:

  - config.html: restored proper indentation on the snyk_code_enabled
    wrapper (lost when the unmanaged-toggle block above it was removed)
  - ldx_sync_config.go, register_configurations.go,
    register_configurations_test.go: gofmt re-aligned columns that
    were widened by SettingSnykOssUnmanagedPrompted's length and not
    re-narrowed by the previous commit
  - oss_test.go: drop the mockResolver.GetBool expectation that the
    feature commit added for the now-removed SettingSnykOssUnmanagedEnabled
    lookup
  - folder_config.go: remove a stray blank line left by the edit
  - register_configurations_test.go: fix the assertion message to
    match origin/main's wording (16 org + 12 folder, not 18+10)

Net diff against origin/main is now only the genuinely new behaviour:
4 files, 62 insertions, 0 deletions (docs + cli_scanner env var + new test).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@snyk-pr-review-bot

Copy link
Copy Markdown

PR Reviewer Guide 🔍

🧪 PR contains tests
🔒 No security concerns identified
⚡ No major issues detected
📚 Repository Context Analyzed

This review considered 11 relevant code sections from 6 files (average relevance: 0.75)

@acke acke added the ⚠️ DONT MERGE Not ready to be merged yet label May 31, 2026
@acke acke changed the title feat(oss): auto-detect C/C++ workspaces and prompt to enable unmanaged scan [IDE-2089] feat(oss): auto-detect C/C++ workspaces and prompt to enable unmanaged scan [IDE-2089] WIP May 31, 2026
@acke acke changed the title feat(oss): auto-detect C/C++ workspaces and prompt to enable unmanaged scan [IDE-2089] WIP refactor(oss): delegate C/C++ auto-detect to CLI extension [IDE-2089] Jun 1, 2026
if env == nil {
env = gotenv.Env{}
}
env["SNYK_AUTODETECT_OSS"] = "1"

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.

[Suggestion] getCommand (infrastructure/cli/cli.go) treats a non-nil env as the complete subprocess environment and skips loading os.Environ(). Today that's safe because updateArgsGetEnvFromSystemAndConfiguration always returns a fully OS-populated map. But the new if env == nil { env = gotenv.Env{} } guard codifies a path that, if ever reached, would hand getCommand a map containing only SNYK_AUTODETECT_OSS=1 — stripping PATH and the rest of the OS env from the CLI subprocess. Consider a one-line comment naming that invariant, or setting the var via the existing AppendCliEnvironmentVariables merge path. Non-blocking.

— AI review

@bastiandoetsch

Copy link
Copy Markdown
Contributor

[Suggestion · non-blocking] SNYK_AUTODETECT_OSS=1 is set unconditionally, but the LS router (shouldUseLegacyScan) sends a default snyk test (no UseOsTest/UseExperimentalRiskScoreInCLI flag) through the legacy CLI path, whereas the architecture doc's diagram routes this var into the os-flows OSWorkflow (the ostest path). Worth confirming whether the legacy CLI also honours the var, or stating explicitly that the feature is gated behind the UseOsTest rollout — and reconciling the mermaid diagram so default-population behaviour is clear. The env var degrades gracefully (ignored if unrecognised), so this is a doc/contract clarification, not a defect.

— AI review

@bastiandoetsch bastiandoetsch left a comment

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.

Reviewed with the full verification pass (semantic, adversarial, security, code-review). The snyk-ls side is a correct, minimal change — set SNYK_AUTODETECT_OSS=1 at the single command-builder choke point so the env reaches both the legacy and ostest paths, with a meaningful test and a genuinely good architecture decision record. No correctness or security issues. Two non-blocking suggestions left inline/below (env-completeness invariant; confirm the var is honoured on the default legacy route). Approving.

— AI review

if env == nil {
env = gotenv.Env{}
}
env["SNYK_AUTODETECT_OSS"] = "1"

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.

Suggestion (non-blocking). The string literal "SNYK_AUTODETECT_OSS" + value "1" is the entire contract between snyk-ls and cli-extension-os-flows, which is the sole arbiter of whether an unmanaged C/C++ scan also runs. That boundary direction is correct (LS only opts in). Two small notes:

  • Consider anchoring this literal with a comment pointing at the extension's gate logic, mirroring the existing precedent for the hard-coded workflow ID in ostest_scan.go.
  • The if env == nil { env = gotenv.Env{} } guard just above is effectively dead in production — the SDK env path (UpdateEnvironmentAndReturnAdditionalParamsGetEnvFromSystemAndConfiguration) always returns a non-nil map; it only fires under test doubles. Harmless, keep or drop.

Verified the feared interactions are non-issues: setting this unconditionally alongside a user-supplied --unmanaged does not double-scan (--unmanaged forces the legacy path where this var is inert), and forcing env non-nil does not strip os.Environ()/PATH (env was already non-nil before this change).

— AI review

@basti-snyk

Copy link
Copy Markdown
Contributor

ℹ️ Automated AI review summary — non-blocking.

Reviewed origin/main...HEAD with four reviewers (semantic, adversarial, security, code-review). No Critical or Should-Fix correctness issues. The production change is a single, well-scoped line (SNYK_AUTODETECT_OSS=1 in prepareScanCommand) with a unit test, and the docs.

Verified the two plausible failure modes are non-issues:

  • Setting the flag unconditionally while the user also passes --unmanaged does not cause a double unmanaged scan — --unmanaged deterministically forces the legacy scan path, where this env var is never read (only the new ostest/os-flows path honours it).
  • Forcing env non-nil does not strip os.Environ()/PATH — the env map was already non-nil in the production SDK flow before this change.

Minor, non-blocking (see inline comment):

  • Cross-repo contract is a bare string literal; consider a comment pointing at the extension's gate logic.
  • The if env == nil guard is dead in production (test-only).
  • Stray unrelated blank line added in Scan (~line 206) — unrelated to IDE-2089; drop to keep the diff surgical.
  • Test-pyramid note: only a unit test asserts the map value; no integration/acceptance test confirms the var reaches the spawned CLI subprocess. A future refactor of the env plumbing could silently drop it. Worth an integration assertion if cheap.

Security: no findings introduced.

— AI review

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

Labels

⚠️ DONT MERGE Not ready to be merged yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants