feat: verify fixedIssueIds/preventedIssueIds against an in-process sc…

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
+}

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
+	}
+}