From dc6ddfd019411805af472bc0e9c9518c681e4e03 Mon Sep 17 00:00:00 2001 From: Victor Ma Date: Fri, 10 Jul 2026 10:00:35 -0400 Subject: [PATCH 1/5] refactor: support enum and object properties in MCP tool schema builder Dedupes the required/not-required branching in createToolFromDefinition and adds enum/object property support, so tool definitions can declare constrained string values and nested object parameters. Unused until a later commit adds schema entries that need it. --- internal/mcp/tools.go | 2 ++ internal/mcp/tools_test.go | 6 +++--- internal/mcp/utils.go | 32 ++++++++++++++------------------ 3 files changed, 19 insertions(+), 21 deletions(-) diff --git a/internal/mcp/tools.go b/internal/mcp/tools.go index cee6dd5..05bd73d 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"` diff --git a/internal/mcp/tools_test.go b/internal/mcp/tools_test.go index 2f4d501..9d79206 100644 --- a/internal/mcp/tools_test.go +++ b/internal/mcp/tools_test.go @@ -2425,9 +2425,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...)) } } From 4786e93f37fa4fc8baf83fe1429ef199becf7f2a Mon Sep 17 00:00:00 2001 From: Victor Ma Date: Fri, 10 Jul 2026 10:02:54 -0400 Subject: [PATCH 2/5] feat: session-scoped correlation ID on feedback analytics events Mint one correlation ID per studio-mcp process (each IDE connection gets a fresh process) and stamp it on every snyk_send_feedback analytics event via NewAnalyticsEventParam, so events emitted by the same session can be joined downstream instead of each call minting its own random ID. --- internal/analytics/analytics.go | 9 +- internal/analytics/analytics_test.go | 127 +++++++++++++++++++++++++++ internal/mcp/llm_binding.go | 20 +++++ internal/mcp/llm_binding_test.go | 42 +++++++++ internal/mcp/tools.go | 2 +- internal/mcp/tools_test.go | 90 +++++++++++++++++++ 6 files changed, 288 insertions(+), 2 deletions(-) create mode 100644 internal/analytics/analytics_test.go diff --git a/internal/analytics/analytics.go b/internal/analytics/analytics.go index 2343160..8022c89 100644 --- a/internal/analytics/analytics.go +++ b/internal/analytics/analytics.go @@ -47,7 +47,13 @@ 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 +77,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..bd6968f 100644 --- a/internal/mcp/llm_binding.go +++ b/internal/mcp/llm_binding.go @@ -62,6 +62,13 @@ 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 (design.md D1). It is + // set once, before the server starts accepting tool calls, and is + // thereafter only read, so it needs no dedicated lock. + correlationID string } func NewMcpLLMBinding(opts ...Option) *McpLLMBinding { @@ -78,8 +85,21 @@ func NewMcpLLMBinding(opts ...Option) *McpLLMBinding { return mcpServerImpl } +// mintCorrelationID mints the session-scoped correlation ID once per process +// (design.md D1), 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/tools.go b/internal/mcp/tools.go index 05bd73d..1f55ffc 100644 --- a/internal/mcp/tools.go +++ b/internal/mcp/tools.go @@ -518,7 +518,7 @@ 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 := analytics.NewAnalyticsEventParam("Send feedback", nil, types.FilePath(path), m.correlationID) event.Extension = buildSendFeedbackExtension(&logger, int(preventedCount), int(remediatedCount), preventedIDs) go analytics.SendAnalytics(invocationCtx.GetEngine(), "", event, nil) diff --git a/internal/mcp/tools_test.go b/internal/mcp/tools_test.go index 9d79206..fa8de19 100644 --- a/internal/mcp/tools_test.go +++ b/internal/mcp/tools_test.go @@ -29,6 +29,7 @@ import ( "reflect" "runtime" "strings" + "sync" "testing" "github.com/golang/mock/gomock" @@ -125,6 +126,59 @@ 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 +1853,42 @@ 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) +} + func TestCoerceStringSlice(t *testing.T) { t.Run("NilInput", func(t *testing.T) { require.Nil(t, coerceStringSlice(nil)) From 1ae7c8a6cd6ba8fc36614c06b3035eb222e2afb9 Mon Sep 17 00:00:00 2001 From: Victor Ma Date: Fri, 10 Jul 2026 10:05:51 -0400 Subject: [PATCH 3/5] feat: verify fixedIssueIds/preventedIssueIds against an in-process scan-result cache Cache the freshest scan findings per file path from successful snyk_code_scan/snyk_sca_scan calls, then check snyk_send_feedback's claimed IDs against that cache and stamp the result (verified/unverifiable/mismatch) on the analytics event. Verification never blocks or delays the feedback call - it's a best-effort, observational check only. --- internal/mcp/llm_binding.go | 12 + internal/mcp/scan_cache.go | 269 ++++++++++++++++ internal/mcp/scan_cache_test.go | 535 ++++++++++++++++++++++++++++++++ internal/mcp/tools.go | 67 +++- internal/mcp/tools_test.go | 201 +++++++++++- 5 files changed, 1069 insertions(+), 15 deletions(-) create mode 100644 internal/mcp/scan_cache.go create mode 100644 internal/mcp/scan_cache_test.go diff --git a/internal/mcp/llm_binding.go b/internal/mcp/llm_binding.go index bd6968f..0e027b0 100644 --- a/internal/mcp/llm_binding.go +++ b/internal/mcp/llm_binding.go @@ -69,6 +69,18 @@ type McpLLMBinding struct { // 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 (design.md D5). + 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 (design.md D2). Keyed by + // file path extracted from each scan's own results, not the tool call's + // path argument. Bounded to maxScanCacheEntries, evicting the + // least-recently-updated entry first (design.md D5). + scanCache map[string]*scanCacheEntry } func NewMcpLLMBinding(opts ...Option) *McpLLMBinding { diff --git a/internal/mcp/scan_cache.go b/internal/mcp/scan_cache.go new file mode 100644 index 0000000..9d55b3c --- /dev/null +++ b/internal/mcp/scan_cache.go @@ -0,0 +1,269 @@ +/* + * © 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 (design.md D5). +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 (design.md D2). 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. Found via manual end-to-end testing (task 6.5): a +// genuinely successful fix was 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 (design.md D4). +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 +// (design.md D4). +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 (design.md D4). 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 (design.md D2: 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..e055b13 --- /dev/null +++ b/internal/mcp/scan_cache_test.go @@ -0,0 +1,535 @@ +/* + * © 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() +} + +// ----------------------------------------------------------------------- +// Task 3.2 / 6.2: cache upsert keyed by file path found in scan results. +// ----------------------------------------------------------------------- + +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 for a bug found via manual end-to-end testing (task + // 6.5): 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, found via manual end-to-end testing (task 6.5): 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 - 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) + }) +} + +// ----------------------------------------------------------------------- +// Task 3.3 / 6.2: 500-entry cap, evicting least-recently-updated first. +// ----------------------------------------------------------------------- + +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") +} + +// ----------------------------------------------------------------------- +// Task 3.4 / 3.5 / 6.3: per-ID verification state and worst-case precedence. +// ----------------------------------------------------------------------- + +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. Per design.md D4, 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/tools.go b/internal/mcp/tools.go index 1f55ffc..b69eb41 100644 --- a/internal/mcp/tools.go +++ b/internal/mcp/tools.go @@ -319,6 +319,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, 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) @@ -510,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 @@ -518,8 +525,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 @@ -542,22 +564,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 fa8de19..72a71a8 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" @@ -1889,6 +1890,172 @@ func TestSnykSendFeedbackHandler_CorrelationID(t *testing.T) { require.Contains(t, string(payloads[1]), expectedURN) } +// TestSnykSendFeedbackHandler_Verification covers task 3.4/3.5 and 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"`) + }) +} + func TestCoerceStringSlice(t *testing.T) { t.Run("NilInput", func(t *testing.T) { require.Nil(t, coerceStringSlice(nil)) @@ -1918,7 +2085,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"] @@ -1929,7 +2096,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") }) @@ -1938,16 +2105,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) { From 9dd43439ddd93b7285f55d9f95fad60dfbcd9f68 Mon Sep 17 00:00:00 2001 From: Victor Ma Date: Fri, 10 Jul 2026 10:07:37 -0400 Subject: [PATCH 4/5] feat: richer optional metrics on snyk_send_feedback Add optional severity/scan-type breakdowns, outcome, breakabilityRisk(+source), strategy, and testsPassed fields to snyk_send_feedback, populated by remediation callers (snyk-fix.md, snyk-batch-fix.md, skills/snyk-fix/SKILL.md) and the secure-at-inception Stop Hook. All fields are optional and additive: counts remain the authoritative metric. --- internal/mcp/llm_binding.go | 12 +-- internal/mcp/scan_cache.go | 22 +++--- internal/mcp/scan_cache_test.go | 17 ++--- internal/mcp/snyk_tools.json | 77 ++++++++++++++++++- internal/mcp/tools.go | 116 +++++++++++++++++++++++++---- internal/mcp/tools_test.go | 126 ++++++++++++++++++++++++++++++-- 6 files changed, 323 insertions(+), 47 deletions(-) diff --git a/internal/mcp/llm_binding.go b/internal/mcp/llm_binding.go index 0e027b0..3f0e968 100644 --- a/internal/mcp/llm_binding.go +++ b/internal/mcp/llm_binding.go @@ -65,21 +65,21 @@ type McpLLMBinding struct { // 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 (design.md D1). It is + // 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 (design.md D5). + // 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 (design.md D2). Keyed by + // verify fixedIssueIds/preventedIssueIds claims. Keyed by // file path extracted from each scan's own results, not the tool call's // path argument. Bounded to maxScanCacheEntries, evicting the - // least-recently-updated entry first (design.md D5). + // least-recently-updated entry first. scanCache map[string]*scanCacheEntry } @@ -97,8 +97,8 @@ func NewMcpLLMBinding(opts ...Option) *McpLLMBinding { return mcpServerImpl } -// mintCorrelationID mints the session-scoped correlation ID once per process -// (design.md D1), if it hasn't been minted already. Every shipped MCP config +// 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. diff --git a/internal/mcp/scan_cache.go b/internal/mcp/scan_cache.go index 9d55b3c..8bd2f60 100644 --- a/internal/mcp/scan_cache.go +++ b/internal/mcp/scan_cache.go @@ -31,7 +31,7 @@ import ( // 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 (design.md D5). +// a tuned production limit. const maxScanCacheEntries = 500 const ( @@ -54,7 +54,7 @@ type scanCacheEntry struct { // 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 (design.md D2). Files the scan didn't report on are left untouched; +// 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. // @@ -68,10 +68,9 @@ type scanCacheEntry struct { // 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. Found via manual end-to-end testing (task 6.5): a -// genuinely successful fix was tagged verification=mismatch because the -// cache never learned its file, then its containing directory, had become -// clean. +// 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 @@ -186,7 +185,7 @@ func (m *McpLLMBinding) evictLeastRecentlyUpdatedLocked() { // 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 (design.md D4). +// checked out against the scan-result cache. type verificationState string const ( @@ -196,8 +195,7 @@ const ( ) // verificationPrecedence orders states from weakest to strongest signal for -// the worst-case reduction below: mismatch > unverifiable > verified -// (design.md D4). +// the worst-case reduction below: mismatch > unverifiable > verified. var verificationPrecedence = map[verificationState]int{ verificationVerified: 1, verificationUnverifiable: 2, @@ -206,7 +204,7 @@ var verificationPrecedence = map[verificationState]int{ // 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 (design.md D4). Returns "" when +// 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 { @@ -229,8 +227,8 @@ func (m *McpLLMBinding) verifyIDs(ids []string) verificationState { // 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 (design.md D2: IDs carry no file information, so there is no -// single "its file" record to check). Callers must hold scanCacheMu. +// 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 { diff --git a/internal/mcp/scan_cache_test.go b/internal/mcp/scan_cache_test.go index e055b13..277dabf 100644 --- a/internal/mcp/scan_cache_test.go +++ b/internal/mcp/scan_cache_test.go @@ -133,10 +133,9 @@ func TestUpdateScanCache_LaterNarrowerScanOverwritesSameFile(t *testing.T) { } func TestUpdateScanCache_CleanSingleFileRescanClearsStaleVulnerableRecord(t *testing.T) { - // Regression test for a bug found via manual end-to-end testing (task - // 6.5): 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 + // 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). @@ -171,9 +170,9 @@ func TestUpdateScanCache_CleanSingleFileRescanClearsStaleVulnerableRecord(t *tes func TestUpdateScanCache_CleanDirectoryRescanClearsStaleVulnerableRecord(t *testing.T) { // Regression test for the directory-scoped counterpart to the single-file - // bug above, found via manual end-to-end testing (task 6.5): 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 + // 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. @@ -379,8 +378,8 @@ func TestVerifyIDs_EmptyListOmitsVerification(t *testing.T) { 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. Per design.md D4, this must - // resolve to unverifiable, never verified. + // 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"})) 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 b69eb41..aade5f6 100644 --- a/internal/mcp/tools.go +++ b/internal/mcp/tools.go @@ -319,7 +319,7 @@ func (m *McpLLMBinding) defaultHandler(invocationCtx workflow.InvocationContext, } } - // Success path: upsert the scan-result cache (design.md D2) before enhancing + // 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) @@ -526,7 +526,7 @@ func (m *McpLLMBinding) snykSendFeedback(invocationCtx workflow.InvocationContex m.updateGafConfigWithIntegrationEnvironment(invocationCtx, clientInfo.Name, clientInfo.Version) - // Verification never blocks or delays the call (design.md D4): it only + // 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)) @@ -536,11 +536,19 @@ func (m *McpLLMBinding) snykSendFeedback(invocationCtx workflow.InvocationContex 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, - verification: verification, + 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: coerceOptionalString(args["outcome"]), + breakabilityRisk: coerceOptionalString(args["breakabilityRisk"]), + breakabilityRiskSource: coerceOptionalString(args["breakabilityRiskSource"]), + strategy: coerceOptionalString(args["strategy"]), + testsPassed: coerceOptionalBoolPtr(args["testsPassed"]), + verification: verification, }) go analytics.SendAnalytics(invocationCtx.GetEngine(), "", event, nil) @@ -564,19 +572,77 @@ func coerceStringSlice(v any) []string { return out } +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 +} + +// coerceOptionalString returns v as a string, or "" when v isn't a string +// (including when it's absent/nil). +func coerceOptionalString(v any) string { + s, _ := v.(string) + return s +} + +// 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 - verification verificationState + 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. +// 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(). @@ -601,6 +667,30 @@ func buildSendFeedbackExtension(logger *zerolog.Logger, p sendFeedbackParams) ma 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 p.verification != "" { ext["mcp::verification"] = string(p.verification) } diff --git a/internal/mcp/tools_test.go b/internal/mcp/tools_test.go index 72a71a8..12daf53 100644 --- a/internal/mcp/tools_test.go +++ b/internal/mcp/tools_test.go @@ -2056,6 +2056,45 @@ func TestSnykSendFeedbackHandler_Verification(t *testing.T) { }) } +// TestSnykSendFeedbackHandler_PreventedIssuesBySeverityLiteralKey guards +// against the specific drift task 6.4 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)) @@ -2131,16 +2170,91 @@ func TestBuildSendFeedbackExtension(t *testing.T) { require.Contains(t, buf.String(), "does not match") }) - t.Run("VerificationIncludedWhenPresent", func(t *testing.T) { - ext := buildSendFeedbackExtension(nil, sendFeedbackParams{preventedCount: 1, verification: verificationVerified}) + 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("VerificationOmittedWhenAbsent", func(t *testing.T) { - ext := buildSendFeedbackExtension(nil, sendFeedbackParams{preventedCount: 1}) - _, has := ext["mcp::verification"] - require.False(t, has) + 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 TestCoerceOptionalString(t *testing.T) { + require.Equal(t, "", coerceOptionalString(nil)) + require.Equal(t, "", coerceOptionalString(42)) + require.Equal(t, "applied", coerceOptionalString("applied")) +} + +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) { From da9bd297e2944856af227a219dac5d04cd95c2ae Mon Sep 17 00:00:00 2001 From: Victor Ma Date: Mon, 13 Jul 2026 10:56:44 -0400 Subject: [PATCH 5/5] refactor: clean up --- Makefile | 2 +- internal/analytics/analytics.go | 13 ++++---- internal/mcp/llm_binding.go | 27 ++++++++------- internal/mcp/scan_cache.go | 59 ++++++++++++++++++--------------- internal/mcp/scan_cache_test.go | 22 +++--------- internal/mcp/tools.go | 37 ++++++++++----------- internal/mcp/tools_test.go | 27 ++++++--------- 7 files changed, 89 insertions(+), 98 deletions(-) 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 8022c89..923f15d 100644 --- a/internal/analytics/analytics.go +++ b/internal/analytics/analytics.go @@ -47,12 +47,13 @@ type EventParam struct { InteractionUUID string `json:"interactionId"` } -// 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. +// 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 { diff --git a/internal/mcp/llm_binding.go b/internal/mcp/llm_binding.go index 3f0e968..161a742 100644 --- a/internal/mcp/llm_binding.go +++ b/internal/mcp/llm_binding.go @@ -65,21 +65,23 @@ type McpLLMBinding struct { // 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 + // 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 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 - // path argument. Bounded to maxScanCacheEntries, evicting the - // least-recently-updated entry first. + // 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 } @@ -98,10 +100,11 @@ func NewMcpLLMBinding(opts ...Option) *McpLLMBinding { } // 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. +// 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() diff --git a/internal/mcp/scan_cache.go b/internal/mcp/scan_cache.go index 8bd2f60..c37bead 100644 --- a/internal/mcp/scan_cache.go +++ b/internal/mcp/scan_cache.go @@ -28,10 +28,11 @@ import ( "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. +// 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 ( @@ -51,26 +52,30 @@ type scanCacheEntry struct { } // 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. +// 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. +// 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 @@ -166,7 +171,8 @@ func isWithinDir(filePath, dir string) bool { } // evictLeastRecentlyUpdatedLocked removes the entry with the oldest updatedAt -// timestamp. Callers must hold scanCacheMu. +// timestamp. +// Callers must hold scanCacheMu. func (m *McpLLMBinding) evictLeastRecentlyUpdatedLocked() { var oldestKey string var oldestTime time.Time @@ -204,9 +210,9 @@ var verificationPrecedence = map[verificationState]int{ // 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. +// 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 "" @@ -228,7 +234,8 @@ func (m *McpLLMBinding) verifyIDs(ids []string) verificationState { // 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. +// record to check). +// Callers must hold scanCacheMu. func (m *McpLLMBinding) verifyIDLocked(id string) verificationState { scanType, ok := scanTypeFromID(id) if !ok { diff --git a/internal/mcp/scan_cache_test.go b/internal/mcp/scan_cache_test.go index 277dabf..8257b04 100644 --- a/internal/mcp/scan_cache_test.go +++ b/internal/mcp/scan_cache_test.go @@ -40,10 +40,6 @@ func newCacheTestBinding() *McpLLMBinding { return NewMcpLLMBinding() } -// ----------------------------------------------------------------------- -// Task 3.2 / 6.2: cache upsert keyed by file path found in scan results. -// ----------------------------------------------------------------------- - func TestUpdateScanCache_SASTKeyedByResultFilePath(t *testing.T) { binding := newCacheTestBinding() toolDef := SnykMcpToolsDefinition{Name: ToolName.CodeTest} @@ -184,8 +180,9 @@ func TestUpdateScanCache_CleanDirectoryRescanClearsStaleVulnerableRecord(t *test 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 - same - // resolution TestUpdateScanCache_SCAKeyedByManifestPath relies on. + // 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" @@ -291,10 +288,6 @@ func TestUpdateScanCache_IgnoresUnrelatedToolsAndMalformedOutput(t *testing.T) { }) } -// ----------------------------------------------------------------------- -// Task 3.3 / 6.2: 500-entry cap, evicting least-recently-updated first. -// ----------------------------------------------------------------------- - func TestScanCacheEviction_501stDistinctFileEvictsLeastRecentlyUpdated(t *testing.T) { binding := newCacheTestBinding() toolDef := SnykMcpToolsDefinition{Name: ToolName.CodeTest} @@ -365,11 +358,6 @@ func TestScanCacheEviction_ReUpsertingExistingFileDoesNotEvict(t *testing.T) { _, hasNewID := entry.ids["sast:javascript/RuleV2"] require.True(t, hasNewID, "the re-scanned file's record must reflect the newer findings") } - -// ----------------------------------------------------------------------- -// Task 3.4 / 3.5 / 6.3: per-ID verification state and worst-case precedence. -// ----------------------------------------------------------------------- - func TestVerifyIDs_EmptyListOmitsVerification(t *testing.T) { binding := newCacheTestBinding() require.Equal(t, verificationState(""), binding.verifyIDs(nil)) @@ -378,8 +366,8 @@ func TestVerifyIDs_EmptyListOmitsVerification(t *testing.T) { 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. + // 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"})) diff --git a/internal/mcp/tools.go b/internal/mcp/tools.go index aade5f6..8a61225 100644 --- a/internal/mcp/tools.go +++ b/internal/mcp/tools.go @@ -543,10 +543,10 @@ func (m *McpLLMBinding) snykSendFeedback(invocationCtx workflow.InvocationContex fixedIssuesBySeverity: coerceBreakdown(args["fixedIssuesBySeverity"], severityBreakdownKeys), preventedIssuesBySeverity: coerceBreakdown(args["preventedIssuesBySeverity"], severityBreakdownKeys), fixedIssuesByScanType: coerceBreakdown(args["fixedIssuesByScanType"], scanTypeBreakdownKeys), - outcome: coerceOptionalString(args["outcome"]), - breakabilityRisk: coerceOptionalString(args["breakabilityRisk"]), - breakabilityRiskSource: coerceOptionalString(args["breakabilityRiskSource"]), - strategy: coerceOptionalString(args["strategy"]), + 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, }) @@ -577,11 +577,12 @@ var ( 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. +// 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 { @@ -603,13 +604,6 @@ func coerceBreakdown(v any, keys []string) map[string]int { return out } -// coerceOptionalString returns v as a string, or "" when v isn't a string -// (including when it's absent/nil). -func coerceOptionalString(v any) string { - s, _ := v.(string) - return s -} - // 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". @@ -639,10 +633,13 @@ type sendFeedbackParams struct { 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. +// 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(). diff --git a/internal/mcp/tools_test.go b/internal/mcp/tools_test.go index 12daf53..382f7b6 100644 --- a/internal/mcp/tools_test.go +++ b/internal/mcp/tools_test.go @@ -155,8 +155,9 @@ func (c *analyticsCapture) all() [][]byte { // 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(). +// 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{} @@ -1890,7 +1891,7 @@ func TestSnykSendFeedbackHandler_CorrelationID(t *testing.T) { require.Contains(t, string(payloads[1]), expectedURN) } -// TestSnykSendFeedbackHandler_Verification covers task 3.4/3.5 and the +// 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 @@ -2057,15 +2058,15 @@ func TestSnykSendFeedbackHandler_Verification(t *testing.T) { } // TestSnykSendFeedbackHandler_PreventedIssuesBySeverityLiteralKey guards -// against the specific drift task 6.4 calls out: the secure-at-inception +// 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. +// 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) @@ -2243,12 +2244,6 @@ func TestCoerceBreakdown(t *testing.T) { }) } -func TestCoerceOptionalString(t *testing.T) { - require.Equal(t, "", coerceOptionalString(nil)) - require.Equal(t, "", coerceOptionalString(42)) - require.Equal(t, "applied", coerceOptionalString("applied")) -} - func TestCoerceOptionalBoolPtr(t *testing.T) { require.Nil(t, coerceOptionalBoolPtr(nil)) require.Nil(t, coerceOptionalBoolPtr("not a bool"))