Skip to content

fix: evaluate Go plugin feature flags early#6660

Closed
mcombuechen wants to merge 1 commit intohotfix/1.1303.2from
feature/go-feature-flags
Closed

fix: evaluate Go plugin feature flags early#6660
mcombuechen wants to merge 1 commit intohotfix/1.1303.2from
feature/go-feature-flags

Conversation

@mcombuechen
Copy link
Contributor

What does this PR do?

Rebased #6645 on hotfix/1.1303.2.

Co-Authored-By: Max <max.combuchen@snyk.io>
@mcombuechen mcombuechen requested review from a team as code owners March 19, 2026 12:29
@snyk-io
Copy link

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

@mihaibuzgau mihaibuzgau reopened this Mar 19, 2026
@snyk-pr-review-bot
Copy link

PR Reviewer Guide 🔍

🧪 PR contains tests
 External call assumptions: `hasFeatureFlagOrDefault` in `src/cli/commands/monitor/index.ts` and `src/lib/snyk-test/index.js` makes a network call to the Snyk API; the code assumes the call will not throw an exception on network failure or timeout, as it has been moved outside of existing error-handling blocks. The context for `isFeatureFlagSupportedForOrg` also suggests an async API call with no explicit failure handling in the new structure.
🔒 No security concerns identified
⚡ Recommended focus areas for review

Interface Regression 🟠 [major]

The properties includeGoStandardLibraryDeps and includePackageUrls have been removed from the configuration interface definition in src/lib/types.ts. However, src/lib/plugins/build-plugin-options.ts still explicitly assigns values to these keys on line 24 and 27. This results in a type mismatch and breaks TypeScript consumers of the Options interface who rely on these Go-specific configuration fields.

configuration?: {
  // Used only with the Go plugin.
  // TODO: remove once UNIFY-891 is done.
  useReplaceName?: boolean;
};
Missing Feature Flag 🟠 [major]

In src/cli/commands/monitor/index.ts, the variable hasPnpmSupport is evaluated early but is never added to the featureFlags Set between lines 255 and 263. In contrast, src/lib/snyk-test/index.js (the corresponding entry point for the test command) does add PNPM_FEATURE_FLAG to the Set. Since the plugin system now relies on this Set for feature resolution, PNPM-specific features will be incorrectly disabled during a monitor scan.

if (includeGoStandardLibraryDeps) {
  featureFlags.add(INCLUDE_GO_STANDARD_LIBRARY_DEPS_FEATURE_FLAG);
}
if (disableGoPackageUrls) {
  featureFlags.add(DISABLE_GO_PACKAGE_URLS_IN_CLI_FEATURE_FLAG);
}
Robustness Regression 🟠 [major]

Feature flag evaluation has been moved outside of the try...catch block that previously protected against resolution errors (e.g., network timeouts or API unavailability). The previous implementation (lines 51-63 in the old hunk) ensured that hasPnpmSupport and other flags defaulted to false if the check failed. The new code allows potential exceptions from hasFeatureFlagOrDefault to propagate, which could crash the CLI during command initialization if the Snyk API is unreachable.

const hasPnpmSupport = await hasFeatureFlagOrDefault(
  PNPM_FEATURE_FLAG,
  options,
);
const includeGoStandardLibraryDeps = await hasFeatureFlagOrDefault(
  INCLUDE_GO_STANDARD_LIBRARY_DEPS_FEATURE_FLAG,
  options,
);
const disableGoPackageUrls = await hasFeatureFlagOrDefault(
  DISABLE_GO_PACKAGE_URLS_IN_CLI_FEATURE_FLAG,
  options,
);
const hasImprovedDotnetWithoutPublish =
  !!options['dotnet-runtime-resolution'] &&
  (await hasFeatureFlagOrDefault(
    DOTNET_WITHOUT_PUBLISH_FEATURE_FLAG,
    options,
  ));
📚 Repository Context Analyzed

This review considered 27 relevant code sections from 11 files (average relevance: 0.98)

@mihaibuzgau
Copy link
Contributor

cherripicked this commit into the hotfix branch: d348cb7. Closing this PR.

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