diff --git a/internal/mcp/llm_binding.go b/internal/mcp/llm_binding.go index b2812ef..d7976d7 100644 --- a/internal/mcp/llm_binding.go +++ b/internal/mcp/llm_binding.go @@ -44,6 +44,13 @@ import ( "github.com/snyk/go-application-framework/pkg/workflow" ) +// 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 ( TransportParam string = "transport" SseTransportType string = "sse" @@ -69,6 +76,24 @@ type McpLLMBinding struct { // 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 scanCacheLocked. + // It is intentionally separate from mutex above, which only guards + // Start/Started lifecycle state. + // + // scanCacheLocked 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. + // + // Do not read or write scanCacheLocked directly; it is lazily + // initialized and mutex-guarded, so all access must go through + // acquireScanCache: + // + // cache, release := m.acquireScanCache() + // defer release() + scanCacheMu sync.Mutex + scanCacheLocked *scanCache } func NewMcpLLMBinding(opts ...Option) *McpLLMBinding { @@ -324,3 +349,77 @@ func (m *McpLLMBinding) addAuthEnvVars(invocationCtx workflow.InvocationContext, return expandedEnv } + +func (m *McpLLMBinding) acquireScanCache() (*scanCache, func()) { + m.scanCacheMu.Lock() + + if m.scanCacheLocked == nil { + m.scanCacheLocked = &scanCache{ + entries: make(map[string]*scanCacheEntry), + maxEntries: MaxScanCacheEntries, + } + } + + return m.scanCacheLocked, m.scanCacheMu.Unlock +} + +// updateScanCache upserts the scan-result cache from a successful scan's +// output. +// +// workDir and scanPath serve different purposes and must not be conflated: +// workDir is the CLI's own working directory (always a directory, used as +// the basePath for resolving the scan output's relative file paths), while +// scanPath is the tool call's own "path" argument exactly as given (a file +// or a directory) and defines the scope the cache treats as authoritative - +// see UpdateSASTIssues/UpdateSCAIssues. Passing workDir for scanPath would +// silently widen a single-file scan's authority to that file's entire +// parent directory. +func (m *McpLLMBinding) updateScanCache(logger *zerolog.Logger, toolDef SnykMcpToolsDefinition, output string, workDir string, scanPath string, includeIgnores bool) { + scanType, issues, err := parseScanOutput(logger, toolDef, output, workDir, includeIgnores) + + 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 + } + + cache, release := m.acquireScanCache() + defer release() + + switch scanType { + case scanTypeSAST: + cache.UpdateSASTIssues(scanPath, issues) + + case scanTypeSCA: + cache.UpdateSCAIssues(scanPath, issues) + + default: + if logger != nil { + logger.Debug().Err(fmt.Errorf("unknown scan type %q", scanType)).Any("scanType", scanType).Msg("Could not update scan cache with unknown scan type") + } + } +} + +// 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 "" + } + + cache, release := m.acquireScanCache() + defer release() + + worst := verificationVerified + for _, id := range ids { + state := cache.VerifyID(id) + if verificationPrecedence[state] > verificationPrecedence[worst] { + worst = state + } + } + return worst +} diff --git a/internal/mcp/scan_cache.go b/internal/mcp/scan_cache.go new file mode 100644 index 0000000..62cff21 --- /dev/null +++ b/internal/mcp/scan_cache.go @@ -0,0 +1,257 @@ +/* + * © 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" + "os" + "path/filepath" + "sort" + "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" +) + +type scanType string + +const ( + scanTypeSAST scanType = "sast" + scanTypeSCA scanType = "sca" +) + +// 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, +} + +// 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 scanType + ids map[string]struct{} + updatedAt time.Time +} + +type scanCache struct { + entries map[string]*scanCacheEntry + + // maxEntries bounds the cache; see MaxScanCacheEntries. + maxEntries int +} + +// UpdateSASTIssues and UpdateSCAIssues upsert one entry per file path found +// in the scan's own results (not the tool call's path argument), fully +// replacing each file's prior record. +// +// scanPath additionally clears stale entries for any file/directory it +// covers that reported no issues, so a clean re-scan can supersede an old +// vulnerable record even though it produces no findings of its own to do +// so. scanPath must be the tool call's own path argument exactly as given +// (a file or a directory) - not a derived working directory, which for a +// single-file call is that file's parent and would wrongly widen this +// scan's authority to every cached sibling file. A single-file scanPath +// clears just that file; a directory scanPath clears every already-cached +// file (of the same scan type) nested under it, matching this tool's +// recursive scan behavior. Without this, a genuine fix could be +// misreported as verification=mismatch because the cache never learned its +// file (or containing directory) had gone clean. +func (s *scanCache) UpdateSASTIssues(scanPath string, issues []types.IssueData) { + now := time.Now() + s.clearEntries(now, scanTypeSAST, scanPath, issues) + s.addEntries(now, scanTypeSAST, issues) +} + +func (s *scanCache) UpdateSCAIssues(scanPath string, issues []types.IssueData) { + now := time.Now() + s.clearEntries(now, scanTypeSCA, scanPath, issues) + s.addEntries(now, scanTypeSCA, issues) +} + +func (s *scanCache) VerifyID(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 s.entries { + if entry.scanType != scanType { + continue + } + foundRelevantScan = true + if _, present := entry.ids[id]; present { + return verificationMismatch + } + } + if !foundRelevantScan { + return verificationUnverifiable + } + + return verificationVerified +} + +// clearEntries implements the scanPath-seeding half of UpdateSASTIssues / +// UpdateSCAIssues described above: it clears stale ids for path itself (or, +// when path is a directory, for every already-cached file of scanType nested +// under it) so that a clean re-scan can supersede a stale vulnerable record +// even when it reports no issues of its own. +func (s *scanCache) clearEntries(now time.Time, scanType scanType, path string, issues []types.IssueData) { + if info, err := os.Stat(path); err == nil { + if info.IsDir() { + for filePath, entry := range s.entries { + if entry.scanType != scanType { + continue + } + + if isWithinDir(filePath, path) { + entry.ids = make(map[string]struct{}) + entry.updatedAt = now + } + } + } else { + if entry, ok := s.entries[path]; ok { + // Only clear an existing entry when it's the same scan type; + // otherwise leave the other scan type's record untouched, + // matching the directory branch's skip logic above. + if entry.scanType == scanType { + entry.ids = make(map[string]struct{}) + entry.updatedAt = now + } + } else { + s.entries[path] = &scanCacheEntry{ + scanType: scanType, + ids: map[string]struct{}{}, + updatedAt: now, + } + } + } + } +} + +func (s *scanCache) addEntries(now time.Time, scanType scanType, issues []types.IssueData) { + // Group issue IDs by file path + prefix := string(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{}{} + } + + // Insert grouped ids (it's ok to over-insert, we will evict excess entries before we return) + for filePath, ids := range idsByFile { + s.entries[filePath] = &scanCacheEntry{ + scanType: scanType, + ids: ids, + updatedAt: now, + } + } + + s.evictExcess() +} + +// evictExcess removes the least-recently-updated entries, oldest first, +// until the cache is back within maxEntries. +func (s *scanCache) evictExcess() { + excess := len(s.entries) - s.maxEntries + if excess <= 0 { + return + } + + keys := make([]string, 0, len(s.entries)) + for key := range s.entries { + keys = append(keys, key) + } + sort.Slice(keys, func(i, j int) bool { + return s.entries[keys[i]].updatedAt.Before(s.entries[keys[j]].updatedAt) + }) + + for _, key := range keys[:excess] { + delete(s.entries, key) + } +} + +func parseScanOutput(logger *zerolog.Logger, toolDef SnykMcpToolsDefinition, output string, workDir string, includeIgnores bool) (scanType, []types.IssueData, error) { + switch toolDef.Name { + case ToolName.CodeTest: + issues, err := code.ConvertSARIFJSONToIssues(logger, []byte(output), workDir, includeIgnores) + return scanTypeSAST, issues, err + case ToolName.ScaTest: + issues, err := oss.ConvertOssJsonToIssues(workDir, []byte(output), includeIgnores) + return scanTypeSCA, issues, err + default: + return "", nil, fmt.Errorf("unsupported tool name %q", toolDef.Name) + } +} + +// 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)) +} + +// 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 scanType, 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..5dbc665 --- /dev/null +++ b/internal/mcp/scan_cache_test.go @@ -0,0 +1,641 @@ +/* + * © 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() +} + +// cacheEntries returns the binding's cached scan entries, tolerating a cache +// that has never been populated (scanCacheLocked stays nil until the first +// successful scan or seeded fixture). +func cacheEntries(b *McpLLMBinding) map[string]*scanCacheEntry { + if b.scanCacheLocked == nil { + return nil + } + return b.scanCacheLocked.entries +} + +// seedCache replaces the binding's cache with the given entries, wiring the +// same eviction cap the production code uses. +func seedCache(b *McpLLMBinding, entries map[string]*scanCacheEntry) { + b.scanCacheLocked = &scanCache{ + maxEntries: MaxScanCacheEntries, + entries: entries, + } +} + +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", "/repo", false) + + binding.scanCacheMu.Lock() + defer binding.scanCacheMu.Unlock() + + require.Len(t, cacheEntries(binding), 2) + + dbEntry, ok := cacheEntries(binding)["/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 := cacheEntries(binding)["/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", "/repo", false) + + binding.scanCacheMu.Lock() + defer binding.scanCacheMu.Unlock() + + require.Len(t, cacheEntries(binding), 1) + entry, ok := cacheEntries(binding)["/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", "/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", "/repo/src", false) + + binding.scanCacheMu.Lock() + defer binding.scanCacheMu.Unlock() + + entry, ok := cacheEntries(binding)["/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, dir, false) + + binding.scanCacheMu.Lock() + _, hasVuln := cacheEntries(binding)[filePath] + binding.scanCacheMu.Unlock() + require.True(t, hasVuln, "sanity check: the vulnerable finding must be cached before the clean re-scan") + + // workDir (the CLI's cwd) stays dir - that's what a single-file scan + // actually gets, since the caller resolves it to the file's parent + // directory - while scanPath is the file itself, exactly as the tool + // call's own path argument named it. Passing dir for scanPath too would + // wrongly widen this scan's authority to every file in dir. + cleanSarif := `{"runs":[{"tool":{"driver":{"rules":[]}},"results":[]}]}` + binding.updateScanCache(&nopLoggerForCache, toolDef, cleanSarif, dir, filePath, false) + + binding.scanCacheMu.Lock() + entry, ok := cacheEntries(binding)[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_CleanSingleFileRescanDoesNotClearOtherScanTypesRecord(t *testing.T) { + // Regression test: the single-file counterpart to + // TestUpdateScanCache_DirectoryRescanDoesNotClearFilesOutsideItsScope. + // entries is keyed only by file path, so a SAST and an SCA record for the + // same path can't coexist as two entries - but a clean single-file re-scan + // of one type must still leave a differently-typed existing record for + // that same path untouched instead of silently clearing its ids (and + // leaving scanType stale) just because the path already had an entry. + binding := newCacheTestBinding() + + dir := t.TempDir() + filePath := filepath.Join(dir, "manifest.mod") + require.NoError(t, os.WriteFile(filePath, []byte("module example\n"), 0o600)) + + binding.scanCacheMu.Lock() + seedCache(binding, map[string]*scanCacheEntry{ + filePath: { + scanType: scanTypeSCA, + ids: map[string]struct{}{"sca:SNYK-OTHER-VULN": {}}, + updatedAt: time.Now(), + }, + }) + binding.scanCacheMu.Unlock() + + toolDef := SnykMcpToolsDefinition{Name: ToolName.CodeTest} + cleanSarif := `{"runs":[{"tool":{"driver":{"rules":[]}},"results":[]}]}` + binding.updateScanCache(&nopLoggerForCache, toolDef, cleanSarif, dir, filePath, false) + + binding.scanCacheMu.Lock() + entry, ok := cacheEntries(binding)[filePath] + binding.scanCacheMu.Unlock() + require.True(t, ok, "the file's pre-existing cache entry must still exist") + require.Equal(t, scanTypeSCA, entry.scanType, "a same-path scan of a different type must not relabel the existing entry's scan type") + _, stillHasVuln := entry.ids["sca:SNYK-OTHER-VULN"] + require.True(t, stillHasVuln, "a clean single-file re-scan must not clear an existing record of a different scan type for the same path") +} + +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, dir, false) + + binding.scanCacheMu.Lock() + _, hasVuln := cacheEntries(binding)[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, dir, false) + + binding.scanCacheMu.Lock() + entry, ok := cacheEntries(binding)[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() + seedCache(binding, 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, scannedDir, false) + + binding.scanCacheMu.Lock() + entry, ok := cacheEntries(binding)[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", "/repo", false) + + binding.scanCacheMu.Lock() + before := cacheEntries(binding)["/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", "/repo", false) + + binding.scanCacheMu.Lock() + after := cacheEntries(binding)["/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", "/repo", false) + binding.scanCacheMu.Lock() + defer binding.scanCacheMu.Unlock() + require.Empty(t, cacheEntries(binding)) + }) + + 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", "/repo", false) + }) + binding.scanCacheMu.Lock() + defer binding.scanCacheMu.Unlock() + require.Empty(t, cacheEntries(binding)) + }) +} + +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", "/repo", false) + } + + binding.scanCacheMu.Lock() + require.Len(t, cacheEntries(binding), MaxScanCacheEntries) + _, leastRecentStillPresent := cacheEntries(binding)["/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", "/repo", false) + + binding.scanCacheMu.Lock() + defer binding.scanCacheMu.Unlock() + + require.Len(t, cacheEntries(binding), MaxScanCacheEntries, "cache must stay capped at MaxScanCacheEntries") + _, evicted := cacheEntries(binding)["/repo/file0.ts"] + require.False(t, evicted, "the least-recently-updated entry must be evicted first") + _, newEntryPresent := cacheEntries(binding)["/repo/file-overflow.ts"] + require.True(t, newEntryPresent) + // A file inserted partway through (neither oldest nor newest) must survive. + _, midEntryPresent := cacheEntries(binding)[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", "/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", "/repo", false) + + binding.scanCacheMu.Lock() + defer binding.scanCacheMu.Unlock() + + require.Len(t, cacheEntries(binding), MaxScanCacheEntries) + entry, ok := cacheEntries(binding)["/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. + seedCache(binding, 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() + seedCache(binding, 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. + seedCache(binding, 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() + seedCache(binding, 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() + seedCache(binding, 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 scanType + 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 := cacheEntries(fixture.binding)[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 = cacheEntries(fixture.binding)[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) +} + +func TestDefaultHandler_ScanCache_SingleFileRescanDoesNotClearSiblingFiles(t *testing.T) { + // Regression test: normalizeParamsAndDetermineWorkingDir resolves a + // single-file "path" argument to that file's *parent directory* for use + // as the CLI's cwd. If that same value were reused as the scan-cache's + // clear scope (as it once was), rescanning just one file clean would be + // wrongly treated as a directory-wide rescan, silently wiping every + // sibling file's real, still-vulnerable cache record. + fixture := setupTestFixture(t) + toolDef := getToolWithName(t, fixture.tools, ToolName.CodeTest) + require.NotNil(t, toolDef) + handler := fixture.binding.defaultHandler(fixture.invocationContext, *toolDef) + tmpDir := t.TempDir() + + dbPath := filepath.Join(tmpDir, "db.ts") + viewPath := filepath.Join(tmpDir, "view.ts") + require.NoError(t, os.WriteFile(dbPath, []byte("// db\n"), 0o600)) + require.NoError(t, os.WriteFile(viewPath, []byte("// view\n"), 0o600)) + + // Seed both files as vulnerable via a directory-wide scan. + bothVulnerableSarif := `{"runs":[{"tool":{"driver":{"rules":[ + {"id":"javascript/SqlInjection","shortDescription":{"text":"SQLi"},"properties":{"categories":["Security"]}}, + {"id":"javascript/XSS","shortDescription":{"text":"XSS"},"properties":{"categories":["Security"]}} + ]}},"results":[ + {"ruleId":"javascript/SqlInjection","level":"error","locations":[{"physicalLocation":{"artifactLocation":{"uri":"db.ts"},"region":{"startLine":1,"startColumn":1}}}]}, + {"ruleId":"javascript/XSS","level":"warning","locations":[{"physicalLocation":{"artifactLocation":{"uri":"view.ts"},"region":{"startLine":1,"startColumn":1}}}]} + ]}]}` + fixture.mockCliOutput(bothVulnerableSarif) + _, err := handler(t.Context(), mcp.CallToolRequest{Params: mcp.CallToolParams{Arguments: map[string]interface{}{"path": tmpDir}}}) + require.NoError(t, err) + + fixture.binding.scanCacheMu.Lock() + _, dbSeeded := cacheEntries(fixture.binding)[dbPath] + _, viewSeeded := cacheEntries(fixture.binding)[viewPath] + fixture.binding.scanCacheMu.Unlock() + require.True(t, dbSeeded, "sanity check: db.ts must be cached as vulnerable before the single-file rescan") + require.True(t, viewSeeded, "sanity check: view.ts must be cached as vulnerable before the single-file rescan") + + // Rescan ONLY db.ts - path is a single file, not the directory - and + // report it clean. + cleanSarif := `{"runs":[{"tool":{"driver":{"rules":[]}},"results":[]}]}` + fixture.mockCliOutput(cleanSarif) + result, err := handler(t.Context(), mcp.CallToolRequest{Params: mcp.CallToolParams{Arguments: map[string]interface{}{"path": dbPath}}}) + require.NoError(t, err) + require.NotNil(t, result) + + fixture.binding.scanCacheMu.Lock() + dbEntry, dbOk := cacheEntries(fixture.binding)[dbPath] + viewEntry, viewOk := cacheEntries(fixture.binding)[viewPath] + fixture.binding.scanCacheMu.Unlock() + + require.True(t, dbOk) + require.Empty(t, dbEntry.ids, "the rescanned file itself must be cleared") + + require.True(t, viewOk, "a sibling file's cache entry must not be wiped by a single-file rescan of a different file") + _, viewStillVuln := viewEntry.ids["sast:javascript/XSS"] + require.True(t, viewStillVuln, "a single-file rescan must only clear the file it actually targeted, not every file in its parent directory") +} diff --git a/internal/mcp/snyk_tools.json b/internal/mcp/snyk_tools.json index dd6cb39..66065e5 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). Counts must NEVER be cumulative. Always send an absolute path.", "command": [], "standardParams": [], "profiles": ["full","lite","experimental"], @@ -750,6 +750,13 @@ "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." } ] }, diff --git a/internal/mcp/tools.go b/internal/mcp/tools.go index 42d056f..9c989f6 100644 --- a/internal/mcp/tools.go +++ b/internal/mcp/tools.go @@ -247,6 +247,23 @@ func (m *McpLLMBinding) runSnyk(ctx context.Context, invocationCtx workflow.Invo return resAsString, nil } +// scanPathArgument returns the tool call's own "path" argument exactly as +// the caller passed it (a file or a directory), for use as the scan-result +// cache's authority scope. This is deliberately distinct from workingDir: +// workingDir is always a directory (it's filepath.Dir(path) when path is a +// file, since that's what the CLI subprocess needs as its cwd), so passing +// workingDir here would silently widen a single-file scan's cache authority +// to that file's entire parent directory. Falls back to workingDir for +// tools with no "path" param. +func scanPathArgument(params map[string]convertedToolParameter, workingDir string) string { + if pathParam, ok := params["path"]; ok { + if pathStr, ok := pathParam.value.(string); ok && pathStr != "" { + return pathStr + } + } + return workingDir +} + // nolint: gocyclo, nolintlint // func is used for all scanners, will be refactored to use GAF WFs // defaultHandler executes a command and enhances output for scan tools func (m *McpLLMBinding) defaultHandler(invocationCtx workflow.InvocationContext, toolDef SnykMcpToolsDefinition) func(ctx context.Context, request mcp.CallToolRequest) (*mcp.CallToolResult, error) { @@ -317,6 +334,12 @@ func (m *McpLLMBinding) defaultHandler(invocationCtx workflow.InvocationContext, } } + // Success path: upsert the scan-result cache (design.md D2) 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, scanPathArgument(params, 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 +531,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 +540,23 @@ func (m *McpLLMBinding) snykSendFeedback(invocationCtx workflow.InvocationContex clientInfo := ClientInfoFromContext(ctx) m.updateGafConfigWithIntegrationEnvironment(invocationCtx, clientInfo.Name, clientInfo.Version) + + // Verification never blocks or delays the call (design.md D4): 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, int(preventedCount), int(remediatedCount), preventedIDs) + event.Extension = buildSendFeedbackExtension(&logger, sendFeedbackParams{ + preventedCount: int(preventedCount), + remediatedCount: int(remediatedCount), + preventedIDs: preventedIDs, + fixedIDs: fixedIDs, + verification: verification, + }) go analytics.SendAnalytics(invocationCtx.GetEngine(), "", event, nil) return mcp.NewToolResultText("Successfully sent feedback"), nil @@ -540,22 +579,45 @@ func coerceStringSlice(v any) []string { return out } +// 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 + verification verificationState +} + // 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 { +// Logs a warning when the supplied preventedIssueIds/fixedIssueIds length disagrees with its +// corresponding count; the count remains authoritative for the metric. +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(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 9fdf591..4e5bba7 100644 --- a/internal/mcp/tools_test.go +++ b/internal/mcp/tools_test.go @@ -31,6 +31,7 @@ import ( "strings" "sync" "testing" + "time" "github.com/golang/mock/gomock" "github.com/mark3labs/mcp-go/mcp" @@ -1890,6 +1891,172 @@ func TestSnykSendFeedbackHandler_CorrelationID(t *testing.T) { 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) + seedCache(fixture.binding, 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"`) + }) +} + func TestCoerceStringSlice(t *testing.T) { t.Run("NilInput", func(t *testing.T) { require.Nil(t, coerceStringSlice(nil)) @@ -1919,7 +2086,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"] @@ -1930,7 +2097,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") }) @@ -1939,16 +2106,42 @@ 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("VerificationIncludedWhenPresent", func(t *testing.T) { + ext := buildSendFeedbackExtension(nil, sendFeedbackParams{preventedCount: 1, verification: verificationVerified}) + require.Equal(t, "verified", ext["mcp::verification"]) + }) + + t.Run("VerificationOmittedWhenAbsent", func(t *testing.T) { + ext := buildSendFeedbackExtension(nil, sendFeedbackParams{preventedCount: 1}) + _, has := ext["mcp::verification"] + require.False(t, has) + }) } func TestHandleFileOutput(t *testing.T) {