diff --git a/Makefile b/Makefile index 9a5daa2..391b251 100644 --- a/Makefile +++ b/Makefile @@ -28,7 +28,7 @@ LDFLAGS_DEV := "-X 'github.com/snyk/studio-mcp/application/config.Development=tr TOOLS_BIN := $(shell pwd)/.bin -OVERRIDE_GOCI_LINT_V := v2.6.1 +OVERRIDE_GOCI_LINT_V := v2.10.1 GOLICENSES_V := v1.6.0 PACT_V := 2.4.2 diff --git a/internal/analytics/analytics.go b/internal/analytics/analytics.go index 2343160..923f15d 100644 --- a/internal/analytics/analytics.go +++ b/internal/analytics/analytics.go @@ -47,7 +47,14 @@ type EventParam struct { InteractionUUID string `json:"interactionId"` } -func NewAnalyticsEventParam(interactionType string, err error, path types.FilePath) EventParam { +// NewAnalyticsEventParam builds a new analytics event. +// correlationID, when non-empty, is stamped on the event as its +// InteractionUUID so that events sharing a correlationID (e.g. a +// session-scoped ID minted once per MCP process) can be joined downstream, +// instead of each call minting its own fresh random ID. +// Pass an empty string to preserve the previous behavior of letting +// PayloadForAnalyticsEventParam mint a fresh UUID for this event. +func NewAnalyticsEventParam(interactionType string, err error, path types.FilePath, correlationID string) EventParam { status := string(analytics.Success) if err != nil { status = string(analytics.Failure) @@ -71,6 +78,7 @@ func NewAnalyticsEventParam(interactionType string, err error, path types.FilePa Status: status, TimestampMs: time.Now().UnixMilli(), TargetId: targetId, + InteractionUUID: correlationID, } } diff --git a/internal/analytics/analytics_test.go b/internal/analytics/analytics_test.go new file mode 100644 index 0000000..7566deb --- /dev/null +++ b/internal/analytics/analytics_test.go @@ -0,0 +1,127 @@ +/* + * © 2025 Snyk Limited + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package analytics + +import ( + "encoding/json" + "testing" + + "github.com/golang/mock/gomock" + "github.com/snyk/go-application-framework/pkg/analytics" + "github.com/snyk/go-application-framework/pkg/configuration" + "github.com/snyk/go-application-framework/pkg/mocks" + "github.com/snyk/go-application-framework/pkg/runtimeinfo" + "github.com/snyk/studio-mcp/internal/types" + "github.com/stretchr/testify/require" +) + +// These tests cover the "Session-scoped correlation ID on feedback events" +// requirement (specs/snyk-fix-feedback-verification): studio-mcp mints one +// correlation ID per process and stamps it on every snyk_send_feedback +// analytics event, replacing the previous behavior of minting a fresh random +// ID on every call. + +func TestNewAnalyticsEventParam_CorrelationID(t *testing.T) { + t.Run("stamps the provided correlation ID as InteractionUUID", func(t *testing.T) { + event := NewAnalyticsEventParam("Send feedback", nil, types.FilePath("/tmp"), "session-correlation-id") + require.Equal(t, "session-correlation-id", event.InteractionUUID) + }) + + t.Run("two calls with the same correlation ID (same session) share it", func(t *testing.T) { + correlationID := "shared-session-id" + first := NewAnalyticsEventParam("Send feedback", nil, types.FilePath("/tmp/a"), correlationID) + second := NewAnalyticsEventParam("Send feedback", nil, types.FilePath("/tmp/b"), correlationID) + + require.Equal(t, correlationID, first.InteractionUUID) + require.Equal(t, first.InteractionUUID, second.InteractionUUID) + }) + + t.Run("different correlation IDs (different sessions/processes) differ", func(t *testing.T) { + first := NewAnalyticsEventParam("Send feedback", nil, types.FilePath("/tmp"), "session-a") + second := NewAnalyticsEventParam("Send feedback", nil, types.FilePath("/tmp"), "session-b") + + require.NotEqual(t, first.InteractionUUID, second.InteractionUUID) + }) + + t.Run("empty correlation ID leaves InteractionUUID unset", func(t *testing.T) { + event := NewAnalyticsEventParam("Send feedback", nil, types.FilePath("/tmp"), "") + require.Empty(t, event.InteractionUUID) + }) +} + +// setupMockEngine builds a minimal mock workflow.Engine sufficient for +// PayloadForAnalyticsEventParam, which only reads GetConfiguration()/GetRuntimeInfo(). +func setupMockEngine(t *testing.T) *mocks.MockEngine { + t.Helper() + ctrl := gomock.NewController(t) + engine := mocks.NewMockEngine(ctrl) + engineConfig := configuration.NewWithOpts(configuration.WithAutomaticEnv()) + engine.EXPECT().GetConfiguration().Return(engineConfig).AnyTimes() + engine.EXPECT().GetRuntimeInfo().Return(runtimeinfo.New(runtimeinfo.WithName("test"), runtimeinfo.WithVersion("1.0.0"))).AnyTimes() + return engine +} + +// interactionURN renders the instrumentation collector's payload to JSON and +// returns it as a string, so tests can assert on the embedded +// "urn:snyk:interaction:" value without reaching into go-application-framework's +// internal API types. +func interactionURNPayload(t *testing.T, engine *mocks.MockEngine, event EventParam) string { + t.Helper() + ic := PayloadForAnalyticsEventParam(engine, "", event) + obj, err := analytics.GetV2InstrumentationObject(ic) + require.NoError(t, err) + b, err := json.Marshal(obj) + require.NoError(t, err) + return string(b) +} + +func TestPayloadForAnalyticsEventParam_CorrelationID(t *testing.T) { + t.Run("preserves a pre-set session correlation ID instead of minting a fresh one", func(t *testing.T) { + engine := setupMockEngine(t) + correlationID := "session-correlation-id" + event := NewAnalyticsEventParam("Send feedback", nil, types.FilePath("/tmp"), correlationID) + + payload := interactionURNPayload(t, engine, event) + + require.Contains(t, payload, "urn:snyk:interaction:"+correlationID) + }) + + t.Run("two events built from the same session correlation ID resolve to the same interaction URN", func(t *testing.T) { + engine := setupMockEngine(t) + correlationID := "shared-session-id" + + eventOne := NewAnalyticsEventParam("Send feedback", nil, types.FilePath("/tmp/a"), correlationID) + eventTwo := NewAnalyticsEventParam("Send feedback", nil, types.FilePath("/tmp/b"), correlationID) + + payloadOne := interactionURNPayload(t, engine, eventOne) + payloadTwo := interactionURNPayload(t, engine, eventTwo) + + expected := "urn:snyk:interaction:" + correlationID + require.Contains(t, payloadOne, expected) + require.Contains(t, payloadTwo, expected) + }) + + t.Run("mints a fresh interaction ID when InteractionUUID is unset", func(t *testing.T) { + engine := setupMockEngine(t) + event := NewAnalyticsEventParam("Send feedback", nil, types.FilePath("/tmp"), "") + require.Empty(t, event.InteractionUUID, "sanity check: no correlation ID was provided") + + payload := interactionURNPayload(t, engine, event) + + require.Contains(t, payload, "urn:snyk:interaction:") + }) +} diff --git a/internal/mcp/llm_binding.go b/internal/mcp/llm_binding.go index 26a4fa4..161a742 100644 --- a/internal/mcp/llm_binding.go +++ b/internal/mcp/llm_binding.go @@ -62,6 +62,27 @@ type McpLLMBinding struct { started bool cliPath string openBrowserFunc types.OpenBrowserFunc + + // correlationID is a session-scoped identifier minted once per process in + // Start() and stamped on every snyk_send_feedback analytics event, so + // events from the same MCP session can be correlated. + // It is set once, before the server starts accepting tool calls, and is + // thereafter only read, so it needs no dedicated lock. + correlationID string + + // scanCacheMu guards scanCache. + // It is intentionally separate from mutex above, which only guards + // Start/Started lifecycle state. + scanCacheMu sync.Mutex + // scanCache is an in-process cache of the freshest scan findings observed + // per file path, populated by defaultHandler after successful + // snyk_code_scan/snyk_sca_scan calls and consulted by snykSendFeedback to + // verify fixedIssueIds/preventedIssueIds claims. + // Keyed by file path extracted from each scan's own results, not the tool + // call's own path argument. + // Bounded to maxScanCacheEntries, evicting the least-recently-updated + // entry first. + scanCache map[string]*scanCacheEntry } func NewMcpLLMBinding(opts ...Option) *McpLLMBinding { @@ -78,8 +99,22 @@ func NewMcpLLMBinding(opts ...Option) *McpLLMBinding { return mcpServerImpl } +// mintCorrelationID mints the session-scoped correlation ID once per process, +// if it hasn't been minted already. +// Every shipped MCP config launches studio-mcp as a fresh process per IDE +// connection, so minting once here (before any tool call can occur) is +// sufficient to correlate every snyk_send_feedback event emitted during this +// session's lifetime. +func (m *McpLLMBinding) mintCorrelationID() { + if m.correlationID == "" { + m.correlationID = uuid.New().String() + } +} + // Start starts the MCP server. It blocks until the server is stopped via Shutdown. func (m *McpLLMBinding) Start(invocationContext workflow.InvocationContext) error { + m.mintCorrelationID() + runTimeInfo := invocationContext.GetRuntimeInfo() version := "" if runTimeInfo != nil { diff --git a/internal/mcp/llm_binding_test.go b/internal/mcp/llm_binding_test.go index 6105aad..c256c48 100644 --- a/internal/mcp/llm_binding_test.go +++ b/internal/mcp/llm_binding_test.go @@ -402,3 +402,45 @@ func TestStart(t *testing.T) { }) }) } + +// TestMintCorrelationID covers the "Session-scoped correlation ID on feedback +// events" requirement (specs/snyk-fix-feedback-verification): one correlation +// ID is minted per process and reused for the rest of that process's +// lifetime, while a different process (a different McpLLMBinding here) gets +// its own distinct ID. +func TestMintCorrelationID(t *testing.T) { + t.Run("mints a non-empty ID", func(t *testing.T) { + binding := NewMcpLLMBinding() + require.Empty(t, binding.correlationID, "correlationID must be unset before minting") + + binding.mintCorrelationID() + + require.NotEmpty(t, binding.correlationID) + }) + + t.Run("two feedback calls in the same process (same binding) share one correlation ID", func(t *testing.T) { + binding := NewMcpLLMBinding() + + binding.mintCorrelationID() + first := binding.correlationID + + // Simulates a second call within the same session/process reading the + // same session-scoped ID; minting must be idempotent. + binding.mintCorrelationID() + second := binding.correlationID + + require.Equal(t, first, second) + }) + + t.Run("two different sessions (different bindings) get different correlation IDs", func(t *testing.T) { + bindingA := NewMcpLLMBinding() + bindingB := NewMcpLLMBinding() + + bindingA.mintCorrelationID() + bindingB.mintCorrelationID() + + require.NotEmpty(t, bindingA.correlationID) + require.NotEmpty(t, bindingB.correlationID) + require.NotEqual(t, bindingA.correlationID, bindingB.correlationID) + }) +} diff --git a/internal/mcp/scan_cache.go b/internal/mcp/scan_cache.go new file mode 100644 index 0000000..c37bead --- /dev/null +++ b/internal/mcp/scan_cache.go @@ -0,0 +1,274 @@ +/* + * © 2025 Snyk Limited + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package mcp + +import ( + "os" + "path/filepath" + "strings" + "time" + + "github.com/rs/zerolog" + "github.com/snyk/studio-mcp/internal/code" + "github.com/snyk/studio-mcp/internal/oss" + "github.com/snyk/studio-mcp/internal/types" +) + +// maxScanCacheEntries caps the scan-result cache at the 500 most-recently +// updated files. +// When a new distinct file path would exceed the cap, the least-recently +// updated entry is evicted first. +// This is a defensive bound, not a tuned production limit. +const maxScanCacheEntries = 500 + +const ( + scanTypeSAST = "sast" + scanTypeSCA = "sca" +) + +// scanCacheEntry is the freshest known finding set for one file path, as +// reported by the most recent snyk_code_scan/snyk_sca_scan call whose results +// included that file. ids are already scan-type-prefixed (e.g. "sast:rule", +// "sca:SNYK-JS-...") so they can be compared directly against +// fixedIssueIds/preventedIssueIds entries. +type scanCacheEntry struct { + scanType string + ids map[string]struct{} + updatedAt time.Time +} + +// updateScanCache upserts one cache entry per file path found in a successful +// scan's own results (SARIF artifactLocation.uri for SAST, the scan's +// reported manifest path (DisplayTargetFile/Path) for SCA), not by the tool +// call's own path argument. +// Both scan types are keyed symmetrically by file path. +// Files the scan didn't report on are left untouched; only files present in +// this scan's results are upserted, and each upsert fully replaces that +// file's previous record with this scan's findings. +// +// The tool call's own workDir argument additionally seeds cache upserts for +// files/directories that a fresh, clean scan covers but reported no issues +// for. +// Without this, a scan that becomes clean could never supersede an earlier +// scan's stale, still-vulnerable record for the same scope, since a scan +// with fewer (or zero) issues produces fewer (or no) idsByFile entries on +// its own. +// When workDir is a single file, that scan is authoritative for that exact +// file. +// When workDir is a directory, that scan is authoritative for every file +// already cached (of the same scan type) within that directory tree, on the +// assumption (true for this tool's default all_projects/recursive scanning) +// that a directory scan comprehensively covers its own scope. +// Without this, a genuinely successful fix could be tagged +// verification=mismatch because the cache never learned its file, then its +// containing directory, had become clean. +func (m *McpLLMBinding) updateScanCache(logger *zerolog.Logger, toolDef SnykMcpToolsDefinition, output string, workDir string, includeIgnores bool) { + var scanType string + var issues []types.IssueData + var err error + + switch toolDef.Name { + case ToolName.CodeTest: + scanType = scanTypeSAST + issues, err = code.ConvertSARIFJSONToIssues(logger, []byte(output), workDir, includeIgnores) + case ToolName.ScaTest: + scanType = scanTypeSCA + issues, err = oss.ConvertOssJsonToIssues(workDir, []byte(output), includeIgnores) + default: + return + } + if err != nil { + if logger != nil { + logger.Debug().Err(err).Str("toolName", toolDef.Name).Msg("Failed to parse scan output for scan-result cache; leaving cache unchanged") + } + return + } + + prefix := scanType + ":" + idsByFile := make(map[string]map[string]struct{}) + for _, issue := range issues { + if issue.FilePath == "" || issue.ID == "" { + continue + } + set, ok := idsByFile[issue.FilePath] + if !ok { + set = make(map[string]struct{}) + idsByFile[issue.FilePath] = set + } + set[prefix+issue.ID] = struct{}{} + } + + m.scanCacheMu.Lock() + defer m.scanCacheMu.Unlock() + + if m.scanCache == nil { + m.scanCache = make(map[string]*scanCacheEntry) + } + + if info, statErr := os.Stat(workDir); statErr == nil { + if info.IsDir() { + for filePath, entry := range m.scanCache { + if entry.scanType != scanType { + continue + } + if _, alreadyCovered := idsByFile[filePath]; alreadyCovered { + continue + } + if isWithinDir(filePath, workDir) { + idsByFile[filePath] = make(map[string]struct{}) + } + } + } else if _, alreadyPresent := idsByFile[workDir]; !alreadyPresent { + idsByFile[workDir] = make(map[string]struct{}) + } + } + + if len(idsByFile) == 0 { + return + } + + now := time.Now() + for filePath, ids := range idsByFile { + if _, exists := m.scanCache[filePath]; !exists && len(m.scanCache) >= maxScanCacheEntries { + m.evictLeastRecentlyUpdatedLocked() + } + m.scanCache[filePath] = &scanCacheEntry{ + scanType: scanType, + ids: ids, + updatedAt: now, + } + } +} + +// isWithinDir reports whether filePath is dir itself or nested inside it, +// comparing cleaned paths so callers don't need to worry about trailing +// separators or relative segments. +func isWithinDir(filePath, dir string) bool { + cleanDir := filepath.Clean(dir) + cleanFile := filepath.Clean(filePath) + if cleanFile == cleanDir { + return true + } + rel, err := filepath.Rel(cleanDir, cleanFile) + if err != nil { + return false + } + return rel != ".." && !strings.HasPrefix(rel, ".."+string(filepath.Separator)) +} + +// evictLeastRecentlyUpdatedLocked removes the entry with the oldest updatedAt +// timestamp. +// Callers must hold scanCacheMu. +func (m *McpLLMBinding) evictLeastRecentlyUpdatedLocked() { + var oldestKey string + var oldestTime time.Time + first := true + for key, entry := range m.scanCache { + if first || entry.updatedAt.Before(oldestTime) { + oldestKey = key + oldestTime = entry.updatedAt + first = false + } + } + if oldestKey != "" { + delete(m.scanCache, oldestKey) + } +} + +// verificationState is the per-call tag attached to a snyk_send_feedback +// analytics event, summarizing how well fixedIssueIds/preventedIssueIds claims +// checked out against the scan-result cache. +type verificationState string + +const ( + verificationVerified verificationState = "verified" + verificationUnverifiable verificationState = "unverifiable" + verificationMismatch verificationState = "mismatch" +) + +// verificationPrecedence orders states from weakest to strongest signal for +// the worst-case reduction below: mismatch > unverifiable > verified. +var verificationPrecedence = map[verificationState]int{ + verificationVerified: 1, + verificationUnverifiable: 2, + verificationMismatch: 3, +} + +// verifyIDs resolves a single event-level verification state for a combined +// list of fixedIssueIds/preventedIssueIds by determining each ID's individual +// state and reducing by worst-case precedence. +// Returns "" when ids is empty, signaling callers should omit the +// verification tag entirely since there is nothing to verify. +func (m *McpLLMBinding) verifyIDs(ids []string) verificationState { + if len(ids) == 0 { + return "" + } + + m.scanCacheMu.Lock() + defer m.scanCacheMu.Unlock() + + worst := verificationVerified + for _, id := range ids { + state := m.verifyIDLocked(id) + if verificationPrecedence[state] > verificationPrecedence[worst] { + worst = state + } + } + return worst +} + +// verifyIDLocked determines the verification state of a single claimed ID by +// scanning the freshest cached record of every cached file of the relevant +// scan type (IDs carry no file information, so there is no single "its file" +// record to check). +// Callers must hold scanCacheMu. +func (m *McpLLMBinding) verifyIDLocked(id string) verificationState { + scanType, ok := scanTypeFromID(id) + if !ok { + // No recognized scan-type prefix (including an empty string or a + // typo'd prefix): there's no relevant scan type to check against. + return verificationUnverifiable + } + + foundRelevantScan := false + for _, entry := range m.scanCache { + if entry.scanType != scanType { + continue + } + foundRelevantScan = true + if _, present := entry.ids[id]; present { + return verificationMismatch + } + } + if !foundRelevantScan { + return verificationUnverifiable + } + return verificationVerified +} + +// scanTypeFromID extracts the scan type from a scan-type-prefixed issue ID +// ("sast:"/"sca:"). ok is false for an empty string or an unrecognized prefix. +func scanTypeFromID(id string) (scanType string, ok bool) { + switch { + case strings.HasPrefix(id, "sast:"): + return scanTypeSAST, true + case strings.HasPrefix(id, "sca:"): + return scanTypeSCA, true + default: + return "", false + } +} diff --git a/internal/mcp/scan_cache_test.go b/internal/mcp/scan_cache_test.go new file mode 100644 index 0000000..8257b04 --- /dev/null +++ b/internal/mcp/scan_cache_test.go @@ -0,0 +1,522 @@ +/* + * © 2025 Snyk Limited + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package mcp + +import ( + "fmt" + "io" + "os" + "path/filepath" + "strings" + "testing" + "time" + + "github.com/mark3labs/mcp-go/mcp" + "github.com/rs/zerolog" + "github.com/stretchr/testify/require" +) + +// nopLoggerForCache is a shared discard logger for cache tests that call +// updateScanCache directly without a full test fixture. +var nopLoggerForCache = zerolog.New(io.Discard) + +// newCacheTestBinding returns a bare McpLLMBinding suitable for exercising +// scan_cache.go directly, without the rest of the tool-call machinery. +func newCacheTestBinding() *McpLLMBinding { + return NewMcpLLMBinding() +} + +func TestUpdateScanCache_SASTKeyedByResultFilePath(t *testing.T) { + binding := newCacheTestBinding() + toolDef := SnykMcpToolsDefinition{Name: ToolName.CodeTest} + + sarif := `{"runs":[{"tool":{"driver":{"rules":[ + {"id":"javascript/SqlInjection","shortDescription":{"text":"SQL Injection"},"properties":{"categories":["Security"]}}, + {"id":"javascript/XSS","shortDescription":{"text":"XSS"},"properties":{"categories":["Security"]}} + ]}},"results":[ + {"ruleId":"javascript/SqlInjection","level":"error","locations":[{"physicalLocation":{"artifactLocation":{"uri":"src/db.ts"},"region":{"startLine":1,"startColumn":1}}}]}, + {"ruleId":"javascript/XSS","level":"warning","locations":[{"physicalLocation":{"artifactLocation":{"uri":"src/view.ts"},"region":{"startLine":2,"startColumn":1}}}]} + ]}]}` + + binding.updateScanCache(&nopLoggerForCache, toolDef, sarif, "/repo", false) + + binding.scanCacheMu.Lock() + defer binding.scanCacheMu.Unlock() + + require.Len(t, binding.scanCache, 2) + + dbEntry, ok := binding.scanCache["/repo/src/db.ts"] + require.True(t, ok, "expected an entry keyed by the SARIF artifactLocation.uri resolved against the scan's basePath") + require.Equal(t, scanTypeSAST, dbEntry.scanType) + _, hasID := dbEntry.ids["sast:javascript/SqlInjection"] + require.True(t, hasID) + + viewEntry, ok := binding.scanCache["/repo/src/view.ts"] + require.True(t, ok) + _, hasID = viewEntry.ids["sast:javascript/XSS"] + require.True(t, hasID) +} + +func TestUpdateScanCache_SCAKeyedByManifestPath(t *testing.T) { + binding := newCacheTestBinding() + toolDef := SnykMcpToolsDefinition{Name: ToolName.ScaTest} + + // displayTargetFile is a lockfile (as the Snyk CLI commonly reports for npm + // projects); getAbsTargetFilePath resolves this to the sibling manifest + // (package.json) joined against the scan's working directory. + scaOutput := `{"ok":false,"displayTargetFile":"package-lock.json","vulnerabilities":[ + {"id":"SNYK-JS-LODASH-1234567","title":"Prototype Pollution","severity":"high","packageName":"lodash","version":"4.17.15","from":["my-app@1.0.0","lodash@4.17.15"],"packageManager":"npm"} + ]}` + + binding.updateScanCache(&nopLoggerForCache, toolDef, scaOutput, "/repo", false) + + binding.scanCacheMu.Lock() + defer binding.scanCacheMu.Unlock() + + require.Len(t, binding.scanCache, 1) + entry, ok := binding.scanCache["/repo/package.json"] + require.True(t, ok, "expected an entry keyed by the SCA scan's reported manifest path") + require.Equal(t, scanTypeSCA, entry.scanType) + _, hasID := entry.ids["sca:SNYK-JS-LODASH-1234567"] + require.True(t, hasID) +} + +func TestUpdateScanCache_LaterNarrowerScanOverwritesSameFile(t *testing.T) { + // "Cache reflects the most recent scan of a file regardless of invocation + // path": a broad scan reports one finding for src/db.ts, then a later, + // narrower scan of the same file reports a different finding set; the + // cache's record for that file must reflect the later scan. + binding := newCacheTestBinding() + toolDef := SnykMcpToolsDefinition{Name: ToolName.CodeTest} + + broadSarif := `{"runs":[{"tool":{"driver":{"rules":[ + {"id":"javascript/SqlInjection","shortDescription":{"text":"SQLi"},"properties":{"categories":["Security"]}} + ]}},"results":[ + {"ruleId":"javascript/SqlInjection","level":"error","locations":[{"physicalLocation":{"artifactLocation":{"uri":"src/db.ts"},"region":{"startLine":1,"startColumn":1}}}]} + ]}]}` + binding.updateScanCache(&nopLoggerForCache, toolDef, broadSarif, "/repo", false) + + narrowerSarif := `{"runs":[{"tool":{"driver":{"rules":[ + {"id":"javascript/XSS","shortDescription":{"text":"XSS"},"properties":{"categories":["Security"]}} + ]}},"results":[ + {"ruleId":"javascript/XSS","level":"warning","locations":[{"physicalLocation":{"artifactLocation":{"uri":"db.ts"},"region":{"startLine":5,"startColumn":1}}}]} + ]}]}` + binding.updateScanCache(&nopLoggerForCache, toolDef, narrowerSarif, "/repo/src", false) + + binding.scanCacheMu.Lock() + defer binding.scanCacheMu.Unlock() + + entry, ok := binding.scanCache["/repo/src/db.ts"] + require.True(t, ok) + _, hasOld := entry.ids["sast:javascript/SqlInjection"] + require.False(t, hasOld, "the stale finding from the broad scan must not survive the later, narrower scan") + _, hasNew := entry.ids["sast:javascript/XSS"] + require.True(t, hasNew, "the cache must reflect the later scan's findings") +} + +func TestUpdateScanCache_CleanSingleFileRescanClearsStaleVulnerableRecord(t *testing.T) { + // Regression test: a genuinely clean re-scan targeted at a single + // just-fixed file must supersede an earlier scan's stale, still-vulnerable + // record for that same file, even though the clean scan reports zero issues (a + // zero-issue result previously produced no idsByFile entries at all, so + // the stale record was never superseded and verification wrongly read + // "mismatch" for a fix that had actually succeeded). + binding := newCacheTestBinding() + toolDef := SnykMcpToolsDefinition{Name: ToolName.CodeTest} + + dir := t.TempDir() + filePath := filepath.Join(dir, "vuln.go") + require.NoError(t, os.WriteFile(filePath, []byte("package main\n"), 0o600)) + + vulnerableSarif := `{"runs":[{"tool":{"driver":{"rules":[ + {"id":"go/CommandInjection","shortDescription":{"text":"Command Injection"},"properties":{"categories":["Security"]}} + ]}},"results":[ + {"ruleId":"go/CommandInjection","level":"error","locations":[{"physicalLocation":{"artifactLocation":{"uri":"vuln.go"},"region":{"startLine":1,"startColumn":1}}}]} + ]}]}` + binding.updateScanCache(&nopLoggerForCache, toolDef, vulnerableSarif, dir, false) + + binding.scanCacheMu.Lock() + _, hasVuln := binding.scanCache[filePath] + binding.scanCacheMu.Unlock() + require.True(t, hasVuln, "sanity check: the vulnerable finding must be cached before the clean re-scan") + + cleanSarif := `{"runs":[{"tool":{"driver":{"rules":[]}},"results":[]}]}` + binding.updateScanCache(&nopLoggerForCache, toolDef, cleanSarif, filePath, false) + + binding.scanCacheMu.Lock() + entry, ok := binding.scanCache[filePath] + binding.scanCacheMu.Unlock() + require.True(t, ok, "the file's cache entry must still exist after the clean re-scan") + require.Empty(t, entry.ids, "a clean single-file re-scan must clear the stale vulnerable record") +} + +func TestUpdateScanCache_CleanDirectoryRescanClearsStaleVulnerableRecord(t *testing.T) { + // Regression test for the directory-scoped counterpart to the single-file + // bug above: an SCA validation re-scan targeted at the whole project + // directory (the normal shape for snyk_sca_scan, unlike a single-file + // SAST re-scan) reported + // zero issues after a dependency fix, but the discovery scan's stale + // go.mod finding was never cleared because a directory-targeted, + // zero-issue scan produced no idsByFile entries to upsert at all. + binding := newCacheTestBinding() + toolDef := SnykMcpToolsDefinition{Name: ToolName.ScaTest} + + dir := t.TempDir() + manifestPath := filepath.Join(dir, "go.mod") + require.NoError(t, os.WriteFile(manifestPath, []byte("module example\n"), 0o600)) + + // displayTargetFile is the lockfile (go.sum), which getAbsTargetFilePath + // resolves to the sibling manifest (go.mod) joined against workDir. + // This is the same resolution TestUpdateScanCache_SCAKeyedByManifestPath + // relies on. + vulnerableOssJSON := `{ + "vulnerabilities": [{"id":"SNYK-GOLANG-STDNETHTTP-16535158","packageName":"std/net/http","version":"1.26.0"}], + "displayTargetFile": "go.sum" + }` + binding.updateScanCache(&nopLoggerForCache, toolDef, vulnerableOssJSON, dir, false) + + binding.scanCacheMu.Lock() + _, hasVuln := binding.scanCache[manifestPath] + binding.scanCacheMu.Unlock() + require.True(t, hasVuln, "sanity check: the vulnerable finding must be cached before the clean re-scan") + + cleanOssJSON := `{"vulnerabilities": [], "displayTargetFile": "go.sum"}` + binding.updateScanCache(&nopLoggerForCache, toolDef, cleanOssJSON, dir, false) + + binding.scanCacheMu.Lock() + entry, ok := binding.scanCache[manifestPath] + binding.scanCacheMu.Unlock() + require.True(t, ok, "the manifest's cache entry must still exist after the clean directory re-scan") + require.Empty(t, entry.ids, "a clean directory re-scan must clear the stale vulnerable record for files within it") +} + +func TestUpdateScanCache_DirectoryRescanDoesNotClearFilesOutsideItsScope(t *testing.T) { + // A directory scan must only clear cached entries within its own scope - + // a cached file under an unrelated directory (of the same scan type) + // must be left untouched. + binding := newCacheTestBinding() + toolDef := SnykMcpToolsDefinition{Name: ToolName.ScaTest} + + scannedDir := t.TempDir() + otherDir := t.TempDir() + otherManifest := filepath.Join(otherDir, "go.mod") + + binding.scanCacheMu.Lock() + binding.scanCache = map[string]*scanCacheEntry{ + otherManifest: { + scanType: scanTypeSCA, + ids: map[string]struct{}{"sca:SNYK-OTHER-VULN": {}}, + updatedAt: time.Now(), + }, + } + binding.scanCacheMu.Unlock() + + cleanOssJSON := `{"vulnerabilities": [], "displayTargetFile": "go.mod"}` + binding.updateScanCache(&nopLoggerForCache, toolDef, cleanOssJSON, scannedDir, false) + + binding.scanCacheMu.Lock() + entry, ok := binding.scanCache[otherManifest] + binding.scanCacheMu.Unlock() + require.True(t, ok) + _, stillHasVuln := entry.ids["sca:SNYK-OTHER-VULN"] + require.True(t, stillHasVuln, "a directory scan must not clear cached entries outside its own scope") +} + +func TestUpdateScanCache_FilesNotInScanResultsAreUnaffected(t *testing.T) { + // "Cache is unaffected by scans that never touch a given file." + binding := newCacheTestBinding() + toolDef := SnykMcpToolsDefinition{Name: ToolName.CodeTest} + + first := `{"runs":[{"tool":{"driver":{"rules":[ + {"id":"javascript/SqlInjection","shortDescription":{"text":"SQLi"},"properties":{"categories":["Security"]}} + ]}},"results":[ + {"ruleId":"javascript/SqlInjection","level":"error","locations":[{"physicalLocation":{"artifactLocation":{"uri":"src/other.ts"},"region":{"startLine":1,"startColumn":1}}}]} + ]}]}` + binding.updateScanCache(&nopLoggerForCache, toolDef, first, "/repo", false) + + binding.scanCacheMu.Lock() + before := binding.scanCache["/repo/src/other.ts"] + binding.scanCacheMu.Unlock() + require.NotNil(t, before) + + // A second scan that reports no findings for src/other.ts at all (outside its scope). + second := `{"runs":[{"tool":{"driver":{"rules":[ + {"id":"javascript/XSS","shortDescription":{"text":"XSS"},"properties":{"categories":["Security"]}} + ]}},"results":[ + {"ruleId":"javascript/XSS","level":"warning","locations":[{"physicalLocation":{"artifactLocation":{"uri":"src/unrelated.ts"},"region":{"startLine":1,"startColumn":1}}}]} + ]}]}` + binding.updateScanCache(&nopLoggerForCache, toolDef, second, "/repo", false) + + binding.scanCacheMu.Lock() + after := binding.scanCache["/repo/src/other.ts"] + binding.scanCacheMu.Unlock() + + require.Equal(t, before, after, "entry for a file untouched by the later scan must be left unchanged") +} + +func TestUpdateScanCache_IgnoresUnrelatedToolsAndMalformedOutput(t *testing.T) { + binding := newCacheTestBinding() + + t.Run("non-scan tool is a no-op", func(t *testing.T) { + binding.updateScanCache(&nopLoggerForCache, SnykMcpToolsDefinition{Name: ToolName.Version}, `{"ok":true}`, "/repo", false) + binding.scanCacheMu.Lock() + defer binding.scanCacheMu.Unlock() + require.Empty(t, binding.scanCache) + }) + + t.Run("malformed JSON does not panic and leaves cache unchanged", func(t *testing.T) { + require.NotPanics(t, func() { + binding.updateScanCache(&nopLoggerForCache, SnykMcpToolsDefinition{Name: ToolName.CodeTest}, "not json", "/repo", false) + }) + binding.scanCacheMu.Lock() + defer binding.scanCacheMu.Unlock() + require.Empty(t, binding.scanCache) + }) +} + +func TestScanCacheEviction_501stDistinctFileEvictsLeastRecentlyUpdated(t *testing.T) { + binding := newCacheTestBinding() + toolDef := SnykMcpToolsDefinition{Name: ToolName.CodeTest} + + sarifForFile := func(file string) string { + return fmt.Sprintf(`{"runs":[{"tool":{"driver":{"rules":[ + {"id":"javascript/Rule","shortDescription":{"text":"Rule"},"properties":{"categories":["Security"]}} + ]}},"results":[ + {"ruleId":"javascript/Rule","level":"warning","locations":[{"physicalLocation":{"artifactLocation":{"uri":"%s"},"region":{"startLine":1,"startColumn":1}}}]} + ]}]}`, file) + } + + // Fill the cache to exactly the cap, one distinct file per call so each + // gets a distinct (increasing) updatedAt timestamp. + for i := 0; i < maxScanCacheEntries; i++ { + binding.updateScanCache(&nopLoggerForCache, toolDef, sarifForFile(fmt.Sprintf("file%d.ts", i)), "/repo", false) + } + + binding.scanCacheMu.Lock() + require.Len(t, binding.scanCache, maxScanCacheEntries) + _, leastRecentStillPresent := binding.scanCache["/repo/file0.ts"] + binding.scanCacheMu.Unlock() + require.True(t, leastRecentStillPresent, "cache must not have evicted anything before reaching the cap") + + // One more distinct file pushes the cache over the cap; the + // least-recently-updated entry (file0.ts, inserted first) must be evicted. + binding.updateScanCache(&nopLoggerForCache, toolDef, sarifForFile("file-overflow.ts"), "/repo", false) + + binding.scanCacheMu.Lock() + defer binding.scanCacheMu.Unlock() + + require.Len(t, binding.scanCache, maxScanCacheEntries, "cache must stay capped at maxScanCacheEntries") + _, evicted := binding.scanCache["/repo/file0.ts"] + require.False(t, evicted, "the least-recently-updated entry must be evicted first") + _, newEntryPresent := binding.scanCache["/repo/file-overflow.ts"] + require.True(t, newEntryPresent) + // A file inserted partway through (neither oldest nor newest) must survive. + _, midEntryPresent := binding.scanCache[fmt.Sprintf("/repo/file%d.ts", maxScanCacheEntries/2)] + require.True(t, midEntryPresent) +} + +func TestScanCacheEviction_ReUpsertingExistingFileDoesNotEvict(t *testing.T) { + binding := newCacheTestBinding() + toolDef := SnykMcpToolsDefinition{Name: ToolName.CodeTest} + + sarifForFile := func(file, ruleID string) string { + return fmt.Sprintf(`{"runs":[{"tool":{"driver":{"rules":[ + {"id":"%s","shortDescription":{"text":"Rule"},"properties":{"categories":["Security"]}} + ]}},"results":[ + {"ruleId":"%s","level":"warning","locations":[{"physicalLocation":{"artifactLocation":{"uri":"%s"},"region":{"startLine":1,"startColumn":1}}}]} + ]}]}`, ruleID, ruleID, file) + } + + for i := 0; i < maxScanCacheEntries; i++ { + binding.updateScanCache(&nopLoggerForCache, toolDef, sarifForFile(fmt.Sprintf("file%d.ts", i), "javascript/Rule"), "/repo", false) + } + + // Re-scanning an already-cached file at the cap must not evict anything, + // since the total distinct-file count doesn't grow. + binding.updateScanCache(&nopLoggerForCache, toolDef, sarifForFile("file0.ts", "javascript/RuleV2"), "/repo", false) + + binding.scanCacheMu.Lock() + defer binding.scanCacheMu.Unlock() + + require.Len(t, binding.scanCache, maxScanCacheEntries) + entry, ok := binding.scanCache["/repo/file0.ts"] + require.True(t, ok) + _, hasNewID := entry.ids["sast:javascript/RuleV2"] + require.True(t, hasNewID, "the re-scanned file's record must reflect the newer findings") +} +func TestVerifyIDs_EmptyListOmitsVerification(t *testing.T) { + binding := newCacheTestBinding() + require.Equal(t, verificationState(""), binding.verifyIDs(nil)) + require.Equal(t, verificationState(""), binding.verifyIDs([]string{})) +} + +func TestVerifyIDs_ColdCacheAtSessionStartIsUnverifiable(t *testing.T) { + // No scan has run yet in this process: the cache is entirely empty, not + // just missing the relevant scan type. + // This must resolve to unverifiable, never verified. + binding := newCacheTestBinding() + require.Equal(t, verificationUnverifiable, binding.verifyIDs([]string{"sast:javascript/SqlInjection"})) + require.Equal(t, verificationUnverifiable, binding.verifyIDs([]string{"sca:SNYK-JS-LODASH-1234567"})) +} + +func TestVerifyIDs_MalformedOrUnrecognizedPrefixIsUnverifiable(t *testing.T) { + binding := newCacheTestBinding() + // Seed some cache data so we can be sure it's the prefix, not an empty + // cache, driving the result. + binding.scanCache = map[string]*scanCacheEntry{ + "/repo/src/db.ts": {scanType: scanTypeSAST, ids: map[string]struct{}{"sast:javascript/SqlInjection": {}}, updatedAt: time.Now()}, + } + + require.Equal(t, verificationUnverifiable, binding.verifyIDs([]string{""}), "empty string has no recognized prefix") + require.Equal(t, verificationUnverifiable, binding.verifyIDs([]string{"iac:CKV_AWS_1"}), "unrecognized prefix") + require.Equal(t, verificationUnverifiable, binding.verifyIDs([]string{"sast"}), "missing colon separator") +} + +func TestVerifyIDs_VerifiedWhenAbsentFromEveryRelevantCachedFile(t *testing.T) { + binding := newCacheTestBinding() + binding.scanCache = map[string]*scanCacheEntry{ + "/repo/src/a.ts": {scanType: scanTypeSAST, ids: map[string]struct{}{"sast:javascript/XSS": {}}, updatedAt: time.Now()}, + "/repo/src/b.ts": {scanType: scanTypeSAST, ids: map[string]struct{}{"sast:javascript/CSRF": {}}, updatedAt: time.Now()}, + } + + require.Equal(t, verificationVerified, binding.verifyIDs([]string{"sast:javascript/SqlInjection"})) +} + +func TestVerifyIDs_UnverifiableWhenNoCachedFileOfRelevantScanType(t *testing.T) { + binding := newCacheTestBinding() + // Only SCA data cached; a SAST claim has no relevant scan type to check against. + binding.scanCache = map[string]*scanCacheEntry{ + "/repo/package.json": {scanType: scanTypeSCA, ids: map[string]struct{}{"sca:SNYK-JS-LODASH-1234567": {}}, updatedAt: time.Now()}, + } + + require.Equal(t, verificationUnverifiable, binding.verifyIDs([]string{"sast:javascript/SqlInjection"})) +} + +func TestVerifyIDs_MismatchWhenStillPresentInSomeCachedFile(t *testing.T) { + binding := newCacheTestBinding() + binding.scanCache = map[string]*scanCacheEntry{ + "/repo/src/a.ts": {scanType: scanTypeSAST, ids: map[string]struct{}{"sast:javascript/SqlInjection": {}}, updatedAt: time.Now()}, + } + + require.Equal(t, verificationMismatch, binding.verifyIDs([]string{"sast:javascript/SqlInjection"})) +} + +func TestVerifyIDs_MixedOutcomesResolveByWorstCasePrecedence(t *testing.T) { + newBindingWithSASTCache := func() *McpLLMBinding { + binding := newCacheTestBinding() + binding.scanCache = map[string]*scanCacheEntry{ + "/repo/src/a.ts": {scanType: scanTypeSAST, ids: map[string]struct{}{"sast:javascript/StillThere": {}}, updatedAt: time.Now()}, + } + return binding + } + + t.Run("mismatch beats verified", func(t *testing.T) { + binding := newBindingWithSASTCache() + ids := []string{"sast:javascript/Fixed", "sast:javascript/StillThere"} + require.Equal(t, verificationMismatch, binding.verifyIDs(ids)) + }) + + t.Run("mismatch beats unverifiable", func(t *testing.T) { + binding := newBindingWithSASTCache() + // The sca: claim has no relevant (SCA) cache entries at all, so it's + // individually unverifiable; the sast: claim mismatches. + ids := []string{"sca:SNYK-UNRELATED-0000001", "sast:javascript/StillThere"} + require.Equal(t, verificationMismatch, binding.verifyIDs(ids)) + }) + + t.Run("unverifiable beats verified", func(t *testing.T) { + binding := newBindingWithSASTCache() + // sast:Fixed is individually verified (absent from the only SAST + // record); the sca: claim has no relevant cache entries at all, so + // it's individually unverifiable, which must still win overall. + ids := []string{"sast:javascript/Fixed", "sca:SNYK-JS-UNRELATED-9999999"} + require.Equal(t, verificationUnverifiable, binding.verifyIDs(ids)) + }) +} + +func TestScanTypeFromID(t *testing.T) { + cases := []struct { + id string + wantType string + wantOK bool + }{ + {"sast:javascript/SqlInjection", scanTypeSAST, true}, + {"sca:SNYK-JS-LODASH-1234567", scanTypeSCA, true}, + {"", "", false}, + {"typo:foo", "", false}, + {"SAST:uppercase-not-recognized", "", false}, + } + for _, tc := range cases { + t.Run(tc.id, func(t *testing.T) { + gotType, gotOK := scanTypeFromID(tc.id) + require.Equal(t, tc.wantOK, gotOK) + require.Equal(t, tc.wantType, gotType) + }) + } +} + +// ----------------------------------------------------------------------- +// Integration-level: cache population via the real defaultHandler, exercising +// the actual scan/tool-call path rather than calling updateScanCache directly. +// ----------------------------------------------------------------------- + +func TestDefaultHandler_ScanCache_BroadThenNarrowerScanSameFile(t *testing.T) { + fixture := setupTestFixture(t) + toolDef := getToolWithName(t, fixture.tools, ToolName.CodeTest) + require.NotNil(t, toolDef) + handler := fixture.binding.defaultHandler(fixture.invocationContext, *toolDef) + tmpDir := t.TempDir() + + broadSarif := `{"runs":[{"tool":{"driver":{"rules":[ + {"id":"javascript/SqlInjection","shortDescription":{"text":"SQLi"},"properties":{"categories":["Security"]}} + ]}},"results":[ + {"ruleId":"javascript/SqlInjection","level":"error","locations":[{"physicalLocation":{"artifactLocation":{"uri":"db.ts"},"region":{"startLine":1,"startColumn":1}}}]} + ]}]}` + fixture.mockCliOutput(broadSarif) + + req := mcp.CallToolRequest{Params: mcp.CallToolParams{Arguments: map[string]interface{}{"path": tmpDir}}} + result, err := handler(t.Context(), req) + require.NoError(t, err) + require.NotNil(t, result) + + fixture.binding.scanCacheMu.Lock() + entry, ok := fixture.binding.scanCache[strings.TrimRight(tmpDir, "/")+"/db.ts"] + fixture.binding.scanCacheMu.Unlock() + require.True(t, ok) + _, hasBroad := entry.ids["sast:javascript/SqlInjection"] + require.True(t, hasBroad) + + narrowerSarif := `{"runs":[{"tool":{"driver":{"rules":[ + {"id":"javascript/XSS","shortDescription":{"text":"XSS"},"properties":{"categories":["Security"]}} + ]}},"results":[ + {"ruleId":"javascript/XSS","level":"warning","locations":[{"physicalLocation":{"artifactLocation":{"uri":"db.ts"},"region":{"startLine":9,"startColumn":1}}}]} + ]}]}` + fixture.mockCliOutput(narrowerSarif) + + result, err = handler(t.Context(), req) + require.NoError(t, err) + require.NotNil(t, result) + + fixture.binding.scanCacheMu.Lock() + entry, ok = fixture.binding.scanCache[strings.TrimRight(tmpDir, "/")+"/db.ts"] + fixture.binding.scanCacheMu.Unlock() + require.True(t, ok) + _, hasOld := entry.ids["sast:javascript/SqlInjection"] + require.False(t, hasOld, "the later scan's own real invocation path must supersede the earlier finding") + _, hasNew := entry.ids["sast:javascript/XSS"] + require.True(t, hasNew) +} diff --git a/internal/mcp/snyk_tools.json b/internal/mcp/snyk_tools.json index dd6cb39..b6fa690 100644 --- a/internal/mcp/snyk_tools.json +++ b/internal/mcp/snyk_tools.json @@ -714,7 +714,7 @@ }, { "name": "snyk_send_feedback", - "description": "Report ONLY the delta (this run only) of Snyk issues. Use preventedIssuesCount if the model prevented introducing a vulnerability in new code. Use fixedExistingIssuesCount if the model repaired an issue in existing code. When the calling tool/hook supplies specific Snyk vuln IDs, pass them in preventedIssueIds. Counts must NEVER be cumulative. Always send an absolute path.", + "description": "Report ONLY the delta (this run only) of Snyk issues. Use preventedIssuesCount if the model prevented introducing a vulnerability in new code. Use fixedExistingIssuesCount if the model repaired an issue in existing code. When the calling tool/hook supplies specific Snyk vuln IDs, pass them in preventedIssueIds (prevention) or fixedIssueIds (remediation). Optional richness fields (severity/scan-type breakdowns, outcome, breakabilityRisk, strategy, testsPassed) may be included when already computed by the caller. Counts must NEVER be cumulative. Always send an absolute path.", "command": [], "standardParams": [], "profiles": ["full","lite","experimental"], @@ -750,6 +750,81 @@ "items": { "type": "string" }, "isRequired": false, "description": "Optional list of Snyk vuln IDs that the caller detected and is asking the model to fix. Prefix each entry with scan type: 'sast:' (e.g. 'sast:javascript/SqlInjection') or 'sca:' (e.g. 'sca:SNYK-JS-LODASH-1234567'). When provided, length should match preventedIssuesCount; the count remains authoritative." + }, + { + "name": "fixedIssueIds", + "type": "array", + "items": { "type": "string" }, + "isRequired": false, + "description": "Optional list of Snyk vuln IDs that were fixed in pre-existing code during this run. Prefix each entry with scan type: 'sast:' (e.g. 'sast:javascript/SqlInjection') or 'sca:' (e.g. 'sca:SNYK-JS-LODASH-1234567'). When provided, length should match fixedExistingIssuesCount; the count remains authoritative." + }, + { + "name": "fixedIssuesBySeverity", + "type": "object", + "isRequired": false, + "properties": { + "critical": { "type": "number" }, + "high": { "type": "number" }, + "medium": { "type": "number" }, + "low": { "type": "number" } + }, + "description": "Optional per-severity breakdown (critical/high/medium/low counts) of fixedExistingIssuesCount. Populated by remediation callers (snyk-fix.md, snyk-batch-fix.md, skills/snyk-fix/SKILL.md)." + }, + { + "name": "preventedIssuesBySeverity", + "type": "object", + "isRequired": false, + "properties": { + "critical": { "type": "number" }, + "high": { "type": "number" }, + "medium": { "type": "number" }, + "low": { "type": "number" } + }, + "description": "Optional per-severity breakdown (critical/high/medium/low counts) of preventedIssuesCount. Populated by the secure-at-inception Stop Hook. Distinct from fixedIssuesBySeverity." + }, + { + "name": "fixedIssuesByScanType", + "type": "object", + "isRequired": false, + "properties": { + "sast": { "type": "number" }, + "sca": { "type": "number" } + }, + "description": "Optional scan-type breakdown (sast/sca counts) of the issue IDs named in fixedIssueIds, decomposing fixedExistingIssuesCount. Remediation-only; not populated by the secure-at-inception Stop Hook." + }, + { + "name": "outcome", + "type": "string", + "isRequired": false, + "enum": ["applied", "blocked", "advisory", "no_fix_available", "allowed"], + "description": "Optional outcome of the remediation attempt. Remediation-only; not populated by the secure-at-inception Stop Hook." + }, + { + "name": "breakabilityRisk", + "type": "string", + "isRequired": false, + "enum": ["low", "medium", "high", "unknown"], + "description": "Optional breaking-change risk assessed for the package upgrade used to fix the issue." + }, + { + "name": "breakabilityRiskSource", + "type": "string", + "isRequired": false, + "enum": ["api", "heuristic"], + "description": "Optional source of breakabilityRisk: 'api' when derived from the real Breakability API, 'heuristic' when derived from the semver+usage-analysis fallback." + }, + { + "name": "strategy", + "type": "string", + "isRequired": false, + "enum": ["A", "B", "C"], + "description": "Optional remediation strategy identifier used for the fix." + }, + { + "name": "testsPassed", + "type": "boolean", + "isRequired": false, + "description": "Optional flag indicating whether tests passed after applying the fix." } ] }, diff --git a/internal/mcp/tools.go b/internal/mcp/tools.go index cee6dd5..8a61225 100644 --- a/internal/mcp/tools.go +++ b/internal/mcp/tools.go @@ -107,6 +107,8 @@ type SnykMcpToolParameter struct { Name string `json:"name"` Type string `json:"type"` Items map[string]any `json:"items,omitempty"` + Properties map[string]any `json:"properties,omitempty"` + Enum []string `json:"enum,omitempty"` IsRequired bool `json:"isRequired"` Description string `json:"description"` SupersedesParams []string `json:"supersedesParams"` @@ -317,6 +319,12 @@ func (m *McpLLMBinding) defaultHandler(invocationCtx workflow.InvocationContext, } } + // Success path: upsert the scan-result cache before enhancing + // output, since enhancement re-shapes `output` into the LLM-facing format. + if success && (toolDef.Name == ToolName.CodeTest || toolDef.Name == ToolName.ScaTest) { + m.updateScanCache(&logger, toolDef, output, workingDir, includeIgnores) + } + // Success path: enhance output and handle file output output = m.enhanceOutput(&logger, toolDef, output, success, workingDir, includeIgnores) return m.handleSuccessOutput(invocationCtx, logger, workingDir, toolDef, output) @@ -508,6 +516,7 @@ func (m *McpLLMBinding) snykSendFeedback(invocationCtx workflow.InvocationContex } preventedIDs := coerceStringSlice(args["preventedIssueIds"]) + fixedIDs := coerceStringSlice(args["fixedIssueIds"]) if preventedCount == 0 && remediatedCount == 0 { return mcp.NewToolResultText("No issues to send feedback for"), nil @@ -516,8 +525,31 @@ func (m *McpLLMBinding) snykSendFeedback(invocationCtx workflow.InvocationContex clientInfo := ClientInfoFromContext(ctx) m.updateGafConfigWithIntegrationEnvironment(invocationCtx, clientInfo.Name, clientInfo.Version) - event := analytics.NewAnalyticsEventParam("Send feedback", nil, types.FilePath(path)) - event.Extension = buildSendFeedbackExtension(&logger, int(preventedCount), int(remediatedCount), preventedIDs) + + // Verification never blocks or delays the call: it only + // annotates the emitted event with a best-effort check of the claimed + // IDs against the scan-result cache. + combinedIDs := make([]string, 0, len(preventedIDs)+len(fixedIDs)) + combinedIDs = append(combinedIDs, preventedIDs...) + combinedIDs = append(combinedIDs, fixedIDs...) + verification := m.verifyIDs(combinedIDs) + + event := analytics.NewAnalyticsEventParam("Send feedback", nil, types.FilePath(path), m.correlationID) + event.Extension = buildSendFeedbackExtension(&logger, sendFeedbackParams{ + preventedCount: int(preventedCount), + remediatedCount: int(remediatedCount), + preventedIDs: preventedIDs, + fixedIDs: fixedIDs, + fixedIssuesBySeverity: coerceBreakdown(args["fixedIssuesBySeverity"], severityBreakdownKeys), + preventedIssuesBySeverity: coerceBreakdown(args["preventedIssuesBySeverity"], severityBreakdownKeys), + fixedIssuesByScanType: coerceBreakdown(args["fixedIssuesByScanType"], scanTypeBreakdownKeys), + outcome: mcp.ExtractString(args, "outcome"), + breakabilityRisk: mcp.ExtractString(args, "breakabilityRisk"), + breakabilityRiskSource: mcp.ExtractString(args, "breakabilityRiskSource"), + strategy: mcp.ExtractString(args, "strategy"), + testsPassed: coerceOptionalBoolPtr(args["testsPassed"]), + verification: verification, + }) go analytics.SendAnalytics(invocationCtx.GetEngine(), "", event, nil) return mcp.NewToolResultText("Successfully sent feedback"), nil @@ -540,22 +572,124 @@ func coerceStringSlice(v any) []string { return out } -// buildSendFeedbackExtension builds the analytics event Extension map for snyk_send_feedback. -// Logs a warning when the supplied preventedIssueIds length disagrees with preventedCount; -// the count remains authoritative for the metric. -func buildSendFeedbackExtension(logger *zerolog.Logger, preventedCount, remediatedCount int, preventedIDs []string) map[string]any { - if logger != nil && len(preventedIDs) > 0 && len(preventedIDs) != preventedCount { +var ( + severityBreakdownKeys = []string{"critical", "high", "medium", "low"} + scanTypeBreakdownKeys = []string{"sast", "sca"} +) + +// coerceBreakdown converts a JSON-decoded object argument (`map[string]any`) +// into a `map[string]int`, keeping only the supplied keys and only values +// that decode as numbers. +// Returns nil when the input isn't an object or no recognized key is +// present, so callers can omit the extension key entirely rather than +// sending an empty map. +func coerceBreakdown(v any, keys []string) map[string]int { + raw, ok := v.(map[string]any) + if !ok { + return nil + } + out := make(map[string]int) + for _, key := range keys { + val, exists := raw[key] + if !exists { + continue + } + if f, ok := val.(float64); ok { + out[key] = int(f) + } + } + if len(out) == 0 { + return nil + } + return out +} + +// coerceOptionalBoolPtr returns a pointer to v's bool value, or nil when v +// isn't a bool (including when it's absent/nil), so callers can distinguish +// "not provided" from "explicitly false". +func coerceOptionalBoolPtr(v any) *bool { + b, ok := v.(bool) + if !ok { + return nil + } + return &b +} + +// sendFeedbackParams holds every optional/required argument parsed from a +// snyk_send_feedback call, used to build the analytics event Extension map. +type sendFeedbackParams struct { + preventedCount int + remediatedCount int + preventedIDs []string + fixedIDs []string + fixedIssuesBySeverity map[string]int + preventedIssuesBySeverity map[string]int + fixedIssuesByScanType map[string]int + outcome string + breakabilityRisk string + breakabilityRiskSource string + strategy string + testsPassed *bool + verification verificationState +} + +// buildSendFeedbackExtension builds the analytics event Extension map for +// snyk_send_feedback. +// Logs a warning when the supplied preventedIssueIds/fixedIssueIds length +// disagrees with its corresponding count; the count remains authoritative +// for the metric. +// All richness fields are optional and are only added to the extension map +// when present. +func buildSendFeedbackExtension(logger *zerolog.Logger, p sendFeedbackParams) map[string]any { + if logger != nil && len(p.preventedIDs) > 0 && len(p.preventedIDs) != p.preventedCount { logger.Warn(). - Int("preventedIssuesCount", preventedCount). - Int("preventedIssueIdsLen", len(preventedIDs)). + Int("preventedIssuesCount", p.preventedCount). + Int("preventedIssueIdsLen", len(p.preventedIDs)). Msg("preventedIssueIds length does not match preventedIssuesCount; count is authoritative") } + if logger != nil && len(p.fixedIDs) > 0 && len(p.fixedIDs) != p.remediatedCount { + logger.Warn(). + Int("fixedExistingIssuesCount", p.remediatedCount). + Int("fixedIssueIdsLen", len(p.fixedIDs)). + Msg("fixedIssueIds length does not match fixedExistingIssuesCount; count is authoritative") + } + ext := map[string]any{ - "mcp::preventedIssuesCount": preventedCount, - "mcp::remediatedIssuesCount": remediatedCount, + "mcp::preventedIssuesCount": p.preventedCount, + "mcp::remediatedIssuesCount": p.remediatedCount, + } + if len(p.preventedIDs) > 0 { + ext["mcp::preventedIssueIds"] = p.preventedIDs + } + if len(p.fixedIDs) > 0 { + ext["mcp::fixedIssueIds"] = p.fixedIDs + } + if len(p.fixedIssuesBySeverity) > 0 { + ext["mcp::fixedIssuesBySeverity"] = p.fixedIssuesBySeverity + } + if len(p.preventedIssuesBySeverity) > 0 { + ext["mcp::preventedIssuesBySeverity"] = p.preventedIssuesBySeverity + } + if len(p.fixedIssuesByScanType) > 0 { + ext["mcp::fixedIssuesByScanType"] = p.fixedIssuesByScanType + } + if p.outcome != "" { + ext["mcp::outcome"] = p.outcome + } + if p.breakabilityRisk != "" { + ext["mcp::breakabilityRisk"] = p.breakabilityRisk + } + if p.breakabilityRiskSource != "" { + ext["mcp::breakabilityRiskSource"] = p.breakabilityRiskSource + } + if p.strategy != "" { + ext["mcp::strategy"] = p.strategy + } + if p.testsPassed != nil { + ext["mcp::testsPassed"] = *p.testsPassed } - if len(preventedIDs) > 0 { - ext["mcp::preventedIssueIds"] = preventedIDs + if p.verification != "" { + ext["mcp::verification"] = string(p.verification) } return ext } diff --git a/internal/mcp/tools_test.go b/internal/mcp/tools_test.go index 2f4d501..382f7b6 100644 --- a/internal/mcp/tools_test.go +++ b/internal/mcp/tools_test.go @@ -29,7 +29,9 @@ import ( "reflect" "runtime" "strings" + "sync" "testing" + "time" "github.com/golang/mock/gomock" "github.com/mark3labs/mcp-go/mcp" @@ -125,6 +127,60 @@ func (f *testFixture) mockCliOutput(output string) { createMockSnykCli(f.t, f.snykCliPath, output) } +// analyticsCapture records the raw payload bytes handed to the mock engine's +// InvokeWithInputAndConfig by each background analytics.SendAnalytics call, and +// lets tests block until a known number of dispatches have completed before +// asserting on them. +type analyticsCapture struct { + wg sync.WaitGroup + mu sync.Mutex + payloads [][]byte +} + +func (c *analyticsCapture) add(n int) { + c.wg.Add(n) +} + +func (c *analyticsCapture) wait() { + c.wg.Wait() +} + +func (c *analyticsCapture) all() [][]byte { + c.mu.Lock() + defer c.mu.Unlock() + return append([][]byte{}, c.payloads...) +} + +// allowAnalyticsDispatch wires the fixture's mock engine so that +// snykSendFeedback's `go analytics.SendAnalytics(...)` background call can run +// to completion (GetLogger/GetRuntimeInfo/InvokeWithInputAndConfig are +// otherwise unmocked and would fail the test) and captures the payload each +// dispatch sends. +// Callers must call capture.add(n) before triggering n dispatches, then +// capture.wait() before inspecting capture.all(). +func (f *testFixture) allowAnalyticsDispatch() *analyticsCapture { + f.t.Helper() + capture := &analyticsCapture{} + + nopLogger := zerolog.Nop() + f.mockEngine.EXPECT().GetLogger().Return(&nopLogger).AnyTimes() + f.mockEngine.EXPECT().GetRuntimeInfo().Return(runtimeinfo.New(runtimeinfo.WithName("test"), runtimeinfo.WithVersion("1.0.0"))).AnyTimes() + f.mockEngine.EXPECT().InvokeWithInputAndConfig(gomock.Any(), gomock.Any(), gomock.Any()).DoAndReturn( + func(_ workflow.Identifier, input []workflow.Data, _ configuration.Configuration) ([]workflow.Data, error) { + defer capture.wg.Done() + if len(input) > 0 { + if b, ok := input[0].GetPayload().([]byte); ok { + capture.mu.Lock() + capture.payloads = append(capture.payloads, b) + capture.mu.Unlock() + } + } + return nil, nil + }).AnyTimes() + + return capture +} + func getToolWithName(t *testing.T, tools *SnykMcpTools, toolName string) *SnykMcpToolsDefinition { t.Helper() for _, tool := range tools.Tools { @@ -1799,6 +1855,247 @@ func TestSnykSendFeedbackHandler_Validation(t *testing.T) { }) } +// TestSnykSendFeedbackHandler_CorrelationID covers the "Two feedback calls in +// the same session share a correlation ID" spec scenario end-to-end through +// the actual snykSendFeedback handler, not just the analytics helpers it +// calls into. +func TestSnykSendFeedbackHandler_CorrelationID(t *testing.T) { + fixture := setupTestFixture(t) + toolDef := getToolWithName(t, fixture.tools, ToolName.SendFeedback) + require.NotNil(t, toolDef) + + fixture.binding.mintCorrelationID() + correlationID := fixture.binding.correlationID + require.NotEmpty(t, correlationID) + + capture := fixture.allowAnalyticsDispatch() + handler := fixture.binding.snykSendFeedback(fixture.invocationContext, *toolDef) + + capture.add(2) + for i := 0; i < 2; i++ { + req := mcp.CallToolRequest{Params: mcp.CallToolParams{Arguments: map[string]interface{}{ + "preventedIssuesCount": float64(1), + "fixedExistingIssuesCount": float64(0), + "path": "/tmp", + }}} + result, err := handler(t.Context(), req) + require.NoError(t, err) + require.NotNil(t, result) + } + capture.wait() + + payloads := capture.all() + require.Len(t, payloads, 2) + expectedURN := "urn:snyk:interaction:" + correlationID + require.Contains(t, string(payloads[0]), expectedURN) + require.Contains(t, string(payloads[1]), expectedURN) +} + +// TestSnykSendFeedbackHandler_Verification covers the +// "Verification state on feedback claims" spec requirement: verified, +// unverifiable, mismatch, and the mixed-ID worst-case-precedence scenario, all +// via the actual snykSendFeedback handler, always succeeding regardless of +// the resolved state. +func TestSnykSendFeedbackHandler_Verification(t *testing.T) { + newFixtureWithCache := func(t *testing.T) *testFixture { + fixture := setupTestFixture(t) + fixture.binding.scanCache = map[string]*scanCacheEntry{ + "/repo/src/db.ts": { + scanType: scanTypeSAST, + ids: map[string]struct{}{"sast:javascript/OtherRule": {}}, + updatedAt: time.Now(), + }, + "/repo/package.json": { + scanType: scanTypeSCA, + ids: map[string]struct{}{"sca:SNYK-JS-LODASH-1234567": {}}, + updatedAt: time.Now(), + }, + } + return fixture + } + + t.Run("verified", func(t *testing.T) { + fixture := newFixtureWithCache(t) + toolDef := getToolWithName(t, fixture.tools, ToolName.SendFeedback) + capture := fixture.allowAnalyticsDispatch() + handler := fixture.binding.snykSendFeedback(fixture.invocationContext, *toolDef) + + capture.add(1) + req := mcp.CallToolRequest{Params: mcp.CallToolParams{Arguments: map[string]interface{}{ + "preventedIssuesCount": float64(0), + "fixedExistingIssuesCount": float64(1), + "path": "/repo", + // Absent from the SAST cache entry above -> confirmed fixed. + "fixedIssueIds": []any{"sast:javascript/SqlInjection"}, + }}} + result, err := handler(t.Context(), req) + require.NoError(t, err) + require.NotNil(t, result) + capture.wait() + + payloads := capture.all() + require.Len(t, payloads, 1) + require.Contains(t, string(payloads[0]), `"mcp::verification":"verified"`) + }) + + t.Run("unverifiable: no cached file of the relevant scan type", func(t *testing.T) { + fixture := setupTestFixture(t) // cold cache: no scans have run yet in this process + toolDef := getToolWithName(t, fixture.tools, ToolName.SendFeedback) + capture := fixture.allowAnalyticsDispatch() + handler := fixture.binding.snykSendFeedback(fixture.invocationContext, *toolDef) + + capture.add(1) + req := mcp.CallToolRequest{Params: mcp.CallToolParams{Arguments: map[string]interface{}{ + "preventedIssuesCount": float64(0), + "fixedExistingIssuesCount": float64(1), + "path": "/repo", + "fixedIssueIds": []any{"sast:javascript/SqlInjection"}, + }}} + result, err := handler(t.Context(), req) + require.NoError(t, err) + require.NotNil(t, result) + capture.wait() + + payloads := capture.all() + require.Len(t, payloads, 1) + require.Contains(t, string(payloads[0]), `"mcp::verification":"unverifiable"`) + }) + + t.Run("unverifiable: malformed/unrecognized scan-type prefix", func(t *testing.T) { + fixture := newFixtureWithCache(t) + toolDef := getToolWithName(t, fixture.tools, ToolName.SendFeedback) + capture := fixture.allowAnalyticsDispatch() + handler := fixture.binding.snykSendFeedback(fixture.invocationContext, *toolDef) + + capture.add(1) + req := mcp.CallToolRequest{Params: mcp.CallToolParams{Arguments: map[string]interface{}{ + "preventedIssuesCount": float64(1), + "fixedExistingIssuesCount": float64(0), + "path": "/repo", + "preventedIssueIds": []any{"typo:not-a-real-prefix"}, + }}} + result, err := handler(t.Context(), req) + require.NoError(t, err) + require.NotNil(t, result) + capture.wait() + + payloads := capture.all() + require.Len(t, payloads, 1) + require.Contains(t, string(payloads[0]), `"mcp::verification":"unverifiable"`) + }) + + t.Run("mismatch: claimed ID still present in a cached file's freshest record", func(t *testing.T) { + fixture := newFixtureWithCache(t) + toolDef := getToolWithName(t, fixture.tools, ToolName.SendFeedback) + capture := fixture.allowAnalyticsDispatch() + handler := fixture.binding.snykSendFeedback(fixture.invocationContext, *toolDef) + + capture.add(1) + req := mcp.CallToolRequest{Params: mcp.CallToolParams{Arguments: map[string]interface{}{ + "preventedIssuesCount": float64(0), + "fixedExistingIssuesCount": float64(1), + "path": "/repo", + "fixedIssueIds": []any{"sast:javascript/OtherRule"}, // still present in the cache + }}} + result, err := handler(t.Context(), req) + require.NoError(t, err) + require.NotNil(t, result) + capture.wait() + + payloads := capture.all() + require.Len(t, payloads, 1) + require.Contains(t, string(payloads[0]), `"mcp::verification":"mismatch"`) + }) + + t.Run("mixed per-ID outcomes resolve to mismatch by worst-case precedence", func(t *testing.T) { + fixture := newFixtureWithCache(t) + toolDef := getToolWithName(t, fixture.tools, ToolName.SendFeedback) + capture := fixture.allowAnalyticsDispatch() + handler := fixture.binding.snykSendFeedback(fixture.invocationContext, *toolDef) + + capture.add(1) + req := mcp.CallToolRequest{Params: mcp.CallToolParams{Arguments: map[string]interface{}{ + "preventedIssuesCount": float64(1), + "fixedExistingIssuesCount": float64(2), + "path": "/repo", + // One verified (absent from the SAST cache), one unverifiable + // (unrecognized "iac:" prefix), one mismatch (still present). + "fixedIssueIds": []any{"sast:javascript/SqlInjection", "iac:not-a-scanned-type"}, + "preventedIssueIds": []any{"sast:javascript/OtherRule"}, + }}} + result, err := handler(t.Context(), req) + require.NoError(t, err) + require.NotNil(t, result) + capture.wait() + + payloads := capture.all() + require.Len(t, payloads, 1) + // mismatch > unverifiable > verified: the single mismatched ID above must win. + require.Contains(t, string(payloads[0]), `"mcp::verification":"mismatch"`) + }) + + t.Run("no IDs named: verification tag omitted", func(t *testing.T) { + fixture := newFixtureWithCache(t) + toolDef := getToolWithName(t, fixture.tools, ToolName.SendFeedback) + capture := fixture.allowAnalyticsDispatch() + handler := fixture.binding.snykSendFeedback(fixture.invocationContext, *toolDef) + + capture.add(1) + req := mcp.CallToolRequest{Params: mcp.CallToolParams{Arguments: map[string]interface{}{ + "preventedIssuesCount": float64(1), + "fixedExistingIssuesCount": float64(0), + "path": "/repo", + }}} + result, err := handler(t.Context(), req) + require.NoError(t, err) + require.NotNil(t, result) + capture.wait() + + payloads := capture.all() + require.Len(t, payloads, 1) + require.NotContains(t, string(payloads[0]), `"mcp::verification"`) + }) +} + +// TestSnykSendFeedbackHandler_PreventedIssuesBySeverityLiteralKey guards +// against the specific drift requirement calls out: the secure-at-inception +// Stop Hook (studio-internal) generates `snyk_send_feedback` instruction text +// with the literal argument name "preventedIssuesBySeverity", and this +// handler must parse that exact literal key. +// Unlike TestBuildSendFeedbackExtension, which exercises +// buildSendFeedbackExtension directly via the internal sendFeedbackParams +// struct, this test goes through the actual MCP request args map (the same +// shape an LLM following the Stop Hook's instruction text would send), so a +// rename on either side (without a matching rename here) fails this test. +func TestSnykSendFeedbackHandler_PreventedIssuesBySeverityLiteralKey(t *testing.T) { + fixture := setupTestFixture(t) + toolDef := getToolWithName(t, fixture.tools, ToolName.SendFeedback) + require.NotNil(t, toolDef) + capture := fixture.allowAnalyticsDispatch() + handler := fixture.binding.snykSendFeedback(fixture.invocationContext, *toolDef) + + capture.add(1) + req := mcp.CallToolRequest{Params: mcp.CallToolParams{Arguments: map[string]interface{}{ + "preventedIssuesCount": float64(3), + "fixedExistingIssuesCount": float64(0), + "path": "/repo", + "preventedIssuesBySeverity": map[string]interface{}{ + "critical": float64(1), + "high": float64(2), + }, + }}} + result, err := handler(t.Context(), req) + require.NoError(t, err) + require.NotNil(t, result) + capture.wait() + + payloads := capture.all() + require.Len(t, payloads, 1) + require.Contains(t, string(payloads[0]), `"mcp::preventedIssuesBySeverity"`) + require.Contains(t, string(payloads[0]), `"critical":1`) + require.Contains(t, string(payloads[0]), `"high":2`) +} + func TestCoerceStringSlice(t *testing.T) { t.Run("NilInput", func(t *testing.T) { require.Nil(t, coerceStringSlice(nil)) @@ -1828,7 +2125,7 @@ func TestCoerceStringSlice(t *testing.T) { func TestBuildSendFeedbackExtension(t *testing.T) { t.Run("CountsOnlyNoIDs", func(t *testing.T) { - ext := buildSendFeedbackExtension(nil, 2, 1, nil) + ext := buildSendFeedbackExtension(nil, sendFeedbackParams{preventedCount: 2, remediatedCount: 1}) require.Equal(t, 2, ext["mcp::preventedIssuesCount"]) require.Equal(t, 1, ext["mcp::remediatedIssuesCount"]) _, hasIDs := ext["mcp::preventedIssueIds"] @@ -1839,7 +2136,7 @@ func TestBuildSendFeedbackExtension(t *testing.T) { ids := []string{"sast:javascript/SqlInjection", "sca:SNYK-JS-LODASH-1234567"} var buf bytes.Buffer logger := zerolog.New(&buf) - ext := buildSendFeedbackExtension(&logger, 2, 0, ids) + ext := buildSendFeedbackExtension(&logger, sendFeedbackParams{preventedCount: 2, preventedIDs: ids}) require.Equal(t, ids, ext["mcp::preventedIssueIds"]) require.Empty(t, buf.String(), "no warning expected when length matches count") }) @@ -1848,16 +2145,111 @@ func TestBuildSendFeedbackExtension(t *testing.T) { ids := []string{"sast:a", "sast:b"} var buf bytes.Buffer logger := zerolog.New(&buf) - ext := buildSendFeedbackExtension(&logger, 3, 0, ids) + ext := buildSendFeedbackExtension(&logger, sendFeedbackParams{preventedCount: 3, preventedIDs: ids}) require.Equal(t, ids, ext["mcp::preventedIssueIds"]) require.Contains(t, buf.String(), "does not match") }) t.Run("EmptyIDsSliceOmittedFromExtension", func(t *testing.T) { - ext := buildSendFeedbackExtension(nil, 0, 0, []string{}) + ext := buildSendFeedbackExtension(nil, sendFeedbackParams{preventedIDs: []string{}}) _, hasIDs := ext["mcp::preventedIssueIds"] require.False(t, hasIDs) }) + + t.Run("FixedIssueIdsMirrorsPreventedIssueIds", func(t *testing.T) { + ids := []string{"sast:javascript/SqlInjection"} + ext := buildSendFeedbackExtension(nil, sendFeedbackParams{remediatedCount: 1, fixedIDs: ids}) + require.Equal(t, ids, ext["mcp::fixedIssueIds"]) + }) + + t.Run("FixedIssueIdsCountMismatchLogsWarning", func(t *testing.T) { + ids := []string{"sast:a", "sast:b"} + var buf bytes.Buffer + logger := zerolog.New(&buf) + ext := buildSendFeedbackExtension(&logger, sendFeedbackParams{remediatedCount: 1, fixedIDs: ids}) + require.Equal(t, ids, ext["mcp::fixedIssueIds"]) + require.Contains(t, buf.String(), "does not match") + }) + + t.Run("RichnessFieldsAllOmittedWhenAbsent", func(t *testing.T) { + ext := buildSendFeedbackExtension(nil, sendFeedbackParams{preventedCount: 1}) + for _, key := range []string{ + "mcp::fixedIssuesBySeverity", "mcp::preventedIssuesBySeverity", "mcp::fixedIssuesByScanType", + "mcp::outcome", "mcp::breakabilityRisk", "mcp::breakabilityRiskSource", "mcp::strategy", + "mcp::testsPassed", "mcp::verification", + } { + _, has := ext[key] + require.False(t, has, "%s must be omitted when not provided", key) + } + }) + + t.Run("RichnessFieldsIncludedWhenPresent", func(t *testing.T) { + testsPassed := true + ext := buildSendFeedbackExtension(nil, sendFeedbackParams{ + preventedCount: 1, + fixedIssuesBySeverity: map[string]int{"high": 1}, + preventedIssuesBySeverity: map[string]int{"critical": 2}, + fixedIssuesByScanType: map[string]int{"sast": 1, "sca": 1}, + outcome: "applied", + breakabilityRisk: "low", + breakabilityRiskSource: "api", + strategy: "A", + testsPassed: &testsPassed, + verification: verificationVerified, + }) + require.Equal(t, map[string]int{"high": 1}, ext["mcp::fixedIssuesBySeverity"]) + require.Equal(t, map[string]int{"critical": 2}, ext["mcp::preventedIssuesBySeverity"]) + require.Equal(t, map[string]int{"sast": 1, "sca": 1}, ext["mcp::fixedIssuesByScanType"]) + require.Equal(t, "applied", ext["mcp::outcome"]) + require.Equal(t, "low", ext["mcp::breakabilityRisk"]) + require.Equal(t, "api", ext["mcp::breakabilityRiskSource"]) + require.Equal(t, "A", ext["mcp::strategy"]) + require.Equal(t, true, ext["mcp::testsPassed"]) + require.Equal(t, "verified", ext["mcp::verification"]) + }) + + t.Run("FixedAndPreventedSeverityBreakdownsAreDistinctFields", func(t *testing.T) { + ext := buildSendFeedbackExtension(nil, sendFeedbackParams{ + preventedCount: 1, + remediatedCount: 1, + fixedIssuesBySeverity: map[string]int{"low": 1}, + preventedIssuesBySeverity: map[string]int{"low": 2}, + }) + require.Equal(t, map[string]int{"low": 1}, ext["mcp::fixedIssuesBySeverity"]) + require.Equal(t, map[string]int{"low": 2}, ext["mcp::preventedIssuesBySeverity"]) + }) +} + +func TestCoerceBreakdown(t *testing.T) { + t.Run("NonObjectInput", func(t *testing.T) { + require.Nil(t, coerceBreakdown("not an object", severityBreakdownKeys)) + require.Nil(t, coerceBreakdown(nil, severityBreakdownKeys)) + }) + + t.Run("EmptyObject", func(t *testing.T) { + require.Nil(t, coerceBreakdown(map[string]any{}, severityBreakdownKeys)) + }) + + t.Run("OnlyRecognizedKeysKept", func(t *testing.T) { + got := coerceBreakdown(map[string]any{ + "critical": float64(1), + "unknown": float64(9), + }, severityBreakdownKeys) + require.Equal(t, map[string]int{"critical": 1}, got) + }) + + t.Run("NonNumericValueDropped", func(t *testing.T) { + got := coerceBreakdown(map[string]any{"sast": "not a number", "sca": float64(2)}, scanTypeBreakdownKeys) + require.Equal(t, map[string]int{"sca": 2}, got) + }) +} + +func TestCoerceOptionalBoolPtr(t *testing.T) { + require.Nil(t, coerceOptionalBoolPtr(nil)) + require.Nil(t, coerceOptionalBoolPtr("not a bool")) + got := coerceOptionalBoolPtr(true) + require.NotNil(t, got) + require.True(t, *got) } func TestHandleFileOutput(t *testing.T) { @@ -2425,9 +2817,9 @@ func TestSnykBreakabilityHandler_SuccessfulResponse(t *testing.T) { "type": "breakability", "attributes": map[string]interface{}{ "package_upgrade": map[string]interface{}{ - "name": "express", - "from_version": "4.18.0", - "to_version": "5.0.0", + "name": "express", + "from_version": "4.18.0", + "to_version": "5.0.0", }, "risk_level": tc.riskLevel, "summary": tc.summary, diff --git a/internal/mcp/utils.go b/internal/mcp/utils.go index 3b9a57d..81096ad 100644 --- a/internal/mcp/utils.go +++ b/internal/mcp/utils.go @@ -88,34 +88,30 @@ func buildArg(key string, param convertedToolParameter) string { func createToolFromDefinition(toolDef *SnykMcpToolsDefinition) mcp.Tool { opts := []mcp.ToolOption{mcp.WithDescription(toolDef.Description)} for _, param := range toolDef.Params { + propOpts := []mcp.PropertyOption{mcp.Description(param.Description)} + if param.IsRequired { + propOpts = append(propOpts, mcp.Required()) + } switch param.Type { case "string": - if param.IsRequired { - opts = append(opts, mcp.WithString(param.Name, mcp.Required(), mcp.Description(param.Description))) - } else { - opts = append(opts, mcp.WithString(param.Name, mcp.Description(param.Description))) + if len(param.Enum) > 0 { + propOpts = append(propOpts, mcp.Enum(param.Enum...)) } + opts = append(opts, mcp.WithString(param.Name, propOpts...)) case "boolean": - if param.IsRequired { - opts = append(opts, mcp.WithBoolean(param.Name, mcp.Required(), mcp.Description(param.Description))) - } else { - opts = append(opts, mcp.WithBoolean(param.Name, mcp.Description(param.Description))) - } + opts = append(opts, mcp.WithBoolean(param.Name, propOpts...)) case "number": - if param.IsRequired { - opts = append(opts, mcp.WithNumber(param.Name, mcp.Required(), mcp.Description(param.Description))) - } else { - opts = append(opts, mcp.WithNumber(param.Name, mcp.Description(param.Description))) - } + opts = append(opts, mcp.WithNumber(param.Name, propOpts...)) case "array": - propOpts := []mcp.PropertyOption{mcp.Description(param.Description)} if param.Items != nil { propOpts = append(propOpts, mcp.Items(param.Items)) } - if param.IsRequired { - propOpts = append(propOpts, mcp.Required()) - } opts = append(opts, mcp.WithArray(param.Name, propOpts...)) + case "object": + if param.Properties != nil { + propOpts = append(propOpts, mcp.Properties(param.Properties)) + } + opts = append(opts, mcp.WithObject(param.Name, propOpts...)) } }