fix: keep CLI-owned retry paths at the application layer [IDE-1890] by basti-snyk · Pull Request #7116 · snyk/cli

basti-snyk · 2026-08-12T07:30:37Z

User description

Summary

declare the CLI's unsafe-method retry paths at the CLI application boundary
preserve user-supplied path overrides and keep network retries opt-in
pin go-application-framework to the unmerged ownership change

Dependency

This draft pins go-application-framework commit 102bfe6ddf2e17c02e5e878ab3744dc296071e23 (v0.14.2-0.20260812074653-102bfe6ddf2e) from go-application-framework #686, which carries current GAF main so module resolution stays tidy. Do not merge this PR until #686 merges and releases; the dependency reference must then be updated to the released GAF version.

Tracking: IDE-1890

Verification

targeted CLI application configuration and engine tests pass
golangci-lint run ./... passes
public CLI build passes
Snyk Code reports no new issues
full Go suite was run; unrelated existing assertions in GAF presenter snapshots and CLI instrumentation scrubbing are not caused by this configuration-only diff

PR Type

Enhancement, Tests

Description

Add CLI-specific retry paths for core endpoints.
Update go-application-framework dependency to v0.17.0.
Enhance test coverage for network retry mechanisms.

Diagram Walkthrough

flowchart LR
  A["CLI Core (main.go)"] --> B("Configuration");
  B --> C{"Add Default Retry Paths"};
  A --> D("Update Dependency");
  D --> E("go-application-framework v0.17.0");
  F["Acceptance Tests"] --> G("Test GAF retries");
  F --> H("Test exit code with timeouts");

File Walkthrough

Relevant files

Enhancement

2 files

configuration.go `Define default network request retry allowed paths`	+26/-0
main.go `Register default retry paths in CLI configuration`	+1/-0

Dependencies

4 files

go.mod `Update go-application-framework dependency`	+1/-1
go.sum `Update go-application-framework checksums`	+2/-2
go.mod `Update go-application-framework dependency`	+1/-1
go.sum `Update go-application-framework checksums`	+2/-2

Tests

3 files

gaf-retry-allowed-paths.spec.ts `Add tests for GAF retry on feature flag evaluation`	+132/-0
exitcode.spec.ts `Make exit code timeout test deterministic`	+43/-2
main_test.go `Add unit and integration tests for retry paths`	+43/-0

Miscellaneous

1 files

.gitleaksignore `Add exit code spec to gitleaksignore`	+1/-0

snyk-io · 2026-08-12T07:31:11Z

✅ 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.

basti-snyk · 2026-08-20T15:13:21Z

/describe

snyk-pr-review-bot · 2026-08-20T15:13:54Z

PR Description updated to latest commit (85fed6a)

basti-snyk · 2026-08-20T15:24:27Z

/describe

snyk-pr-review-bot · 2026-08-20T15:25:02Z

PR Description updated to latest commit (85fed6a)

basti-snyk · 2026-08-21T06:41:55Z

/describe

snyk-pr-review-bot · 2026-08-21T06:42:26Z

PR Description updated to latest commit (91fb301)

basti-snyk · 2026-08-21T06:42:59Z

/describe

snyk-pr-review-bot · 2026-08-21T06:43:28Z

PR Description updated to latest commit (91fb301)

robertolopezlopez

Blocking this PR as GAF 0.17.0 changed timeout behaviour.

Once GAF is fixed + released, dependency in CLI is bumped and acceptance CI is happy, we can unlock this

basti-snyk · 2026-08-21T09:41:27Z

/describe

snyk-pr-review-bot · 2026-08-21T09:42:15Z

PR Description updated to latest commit (4d1fec2)

basti-snyk · 2026-08-21T11:51:34Z

/describe

snyk-pr-review-bot · 2026-08-21T11:52:23Z

PR Description updated to latest commit (4d1fec2)

danskmt · 2026-08-21T12:17:13Z

  it('Correct exit code when snyk_timeout_secs expires', async () => {
+    // Response delay exceeds the watchdog's kill window (timeout + grace period), so
+    // the CLI is always force-killed before any response can arrive -- deterministic
+    // regardless of how many retries GAF performs underneath.
+    server.setResponseDelay(10000);
+
    const testEnv = {
-      ...process.env,
-      SNYK_TIMEOUT_SECS: '1',
+      ...baseEnv,
+      SNYK_TIMEOUT_SECS: '5',
    };


Why this change? I don't think this should be changed.
If the test was failing and this was an attempt to get it to pass, please discard these changes. There is an incident in Slack regarding this, and the failure was due to a rollout of another team.

GAF 0.17.0 narrowed its default network-retry allow-list to only oauth2/token. Add the paths the CLI relied on as implicit free defaults before that change (test-dep-graph, verify/token, feature_flags/evaluation) so unsafe-method requests to them keep retrying on transient failures. Also includes go.sum cleanup from `make tidy` removing stale checksums for the superseded go-application-framework v0.16.1. Add an acceptance test proving GAF retries feature_flags/evaluation on a transient failure, and make the snyk_timeout_secs exit-code test deterministic (it previously raced real backend network timing against the retry policy).

basti-snyk · 2026-08-21T12:43:52Z

Update: the acceptance-tests failures blocking this PR (SNYK_TIMEOUT_SECS exit-code test racing real backend timing against GAF's retries) have been root-caused and fixed. test_and_release is now green on this branch — see the latest CI run. Also added a regression test proving the feature_flags/evaluation retry-allowed-path actually works (test/jest/acceptance/snyk-code/gaf-retry-allowed-paths.spec.ts).

@robertolopezlopez the acceptance CI condition you flagged is now satisfied — could you take another look when you have a chance?

snyk-pr-review-bot · 2026-08-21T12:44:14Z

PR Reviewer Guide 🔍

🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review Inconsistent Path Trimming 🟡 [minor] In `defaultNetworkRequestRetryAllowedPaths`, paths provided via a CSV string are trimmed of whitespace using `strings.TrimSpace`, but paths provided via `[]string` or `[]interface{}` (standard for JSON/YAML config files) are appended as-is. If a user defines paths in a config file with leading/trailing spaces, the equality check in the network middleware will fail to match the endpoint, silently disabling retries for those paths. } else if strSlice, ok := existingValue.([]string); ok { paths = append(paths, strSlice...) } else if ifaceSlice, ok := existingValue.([]interface{}); ok { for _, v := range ifaceSlice { if str, ok := v.(string); ok && str != "" { paths = append(paths, str) } } }
📚 Repository Context Analyzed This review considered 13 relevant code sections from 7 files (average relevance: 0.90) CLAUDE.md (3 segments, lines 1-94) cliv2/pkg/core/main.go (4 segments, lines 1-188) cliv2/internal/constants/constants.go (lines 1-44) package.json (lines 1-236) scripts/upgrade-snyk-go-dependencies.go (lines 1-149) package-lock.json (2 segments, lines 6-266) cliv2/pkg/core/configuration.go (lines 1-56)
🤖 Repository instructions applied (from `AGENTS.md`)

graphite-app · 2026-08-21T12:46:19Z

+		} else if strSlice, ok := existingValue.([]string); ok {
+			paths = append(paths, strSlice...)


Empty string filtering is inconsistent across input types. The CSV string handler filters empty strings (if trimmed != ""), and the interface slice handler filters them (if str != ""), but the string slice handler does not filter empty strings.

If a user provides []string{"a", "", "b"} via configuration, the empty string will be added to the retry paths, which could cause GAF's retry middleware to incorrectly match all paths (depending on how it handles empty path segments).

Fix:

} else if strSlice, ok := existingValue.([]string); ok { for _, s := range strSlice { if s != "" { paths = append(paths, s) } } }

This ensures consistent empty string filtering across all input type handlers.

Suggested change

} else if strSlice, ok := existingValue.([]string); ok {

paths = append(paths, strSlice...)

} else if strSlice, ok := existingValue.([]string); ok {

for _, s := range strSlice {

if s != "" {

paths = append(paths, s)

}

}

Spotted by Graphite

Is this helpful? React 👍 or 👎 to let us know.

danskmt · 2026-08-21T13:02:27Z

Update: the acceptance-tests failures blocking this PR (SNYK_TIMEOUT_SECS exit-code test racing real backend timing against GAF's retries) have been root-caused and fixed. test_and_release is now green on this branch — see the latest CI run. Also added a regression test proving the feature_flags/evaluation retry-allowed-path actually works (test/jest/acceptance/snyk-code/gaf-retry-allowed-paths.spec.ts).

@robertolopezlopez the acceptance CI condition you flagged is now satisfied — could you take another look when you have a chance?

@basti-snyk please remove unrelated changes (I might be mistaken and they might be required)

robertolopezlopez · 2026-08-21T13:42:56Z

@robertolopezlopez the acceptance CI condition you flagged is now satisfied — could you take another look when you have a chance?

@basti-snyk it is actually failing (a couple of linux arm64 acceptance tests, maybe ephemeral failures)

robertolopezlopez · 2026-08-21T13:43:20Z

@basti-snyk also, the branch is off-sync with main

basti-snyk added the ⚠️ DO NOT MERGE ⚠️ label Aug 12, 2026

basti-snyk force-pushed the feat/IDE-1890-cli-retry-path-defaults branch 2 times, most recently from fea402b to 15e0c70 Compare August 12, 2026 08:14

basti-snyk force-pushed the feat/IDE-1890-cli-retry-path-defaults branch from 15e0c70 to 3b2e3c8 Compare August 20, 2026 14:22

basti-snyk removed the ⚠️ DO NOT MERGE ⚠️ label Aug 20, 2026

basti-snyk marked this pull request as ready for review August 21, 2026 06:37

basti-snyk requested a review from a team as a code owner August 21, 2026 06:37