diff --git a/internal/mcp/snyk_tools.json b/internal/mcp/snyk_tools.json index 0d4792b..1a82275 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. 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. Counts must NEVER be cumulative. Always send an absolute path.", "command": [], "standardParams": [], "profiles": ["full","lite","experimental"], @@ -743,6 +743,13 @@ "type": "string", "isRequired": true, "description": "Absolute path to the project root or subdirectory. Example: '/a/my-project' (Linux/macOS) or 'C:\\a\\my-project' (Windows)." + }, + { + "name": "preventedIssueIds", + "type": "array", + "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." } ] }, diff --git a/internal/mcp/tools.go b/internal/mcp/tools.go index 4c48035..a9e1531 100644 --- a/internal/mcp/tools.go +++ b/internal/mcp/tools.go @@ -104,13 +104,14 @@ type SnykMcpToolsDefinition struct { } type SnykMcpToolParameter struct { - Name string `json:"name"` - Type string `json:"type"` - IsRequired bool `json:"isRequired"` - Description string `json:"description"` - SupersedesParams []string `json:"supersedesParams"` - IsPositional bool `json:"isPositional"` - Position int `json:"position"` + Name string `json:"name"` + Type string `json:"type"` + Items map[string]any `json:"items,omitempty"` + IsRequired bool `json:"isRequired"` + Description string `json:"description"` + SupersedesParams []string `json:"supersedesParams"` + IsPositional bool `json:"isPositional"` + Position int `json:"position"` } //go:embed snyk_tools.json @@ -484,18 +485,17 @@ func (m *McpLLMBinding) snykSendFeedback(invocationCtx workflow.InvocationContex logger := m.logger.With().Str("method", toolDef.Name).Logger() logger.Debug().Str("toolName", toolDef.Name).Msg("Received call for tool") - preventedCountStr := request.GetArguments()["preventedIssuesCount"] - remediatedCountStr := request.GetArguments()["fixedExistingIssuesCount"] + args := request.GetArguments() - preventedCount, ok := preventedCountStr.(float64) + preventedCount, ok := args["preventedIssuesCount"].(float64) if !ok { return nil, fmt.Errorf("invalid argument preventedIssuesCount") } - remediatedCount, ok := remediatedCountStr.(float64) + remediatedCount, ok := args["fixedExistingIssuesCount"].(float64) if !ok { return nil, fmt.Errorf("invalid argument fixedExistingIssuesCount") } - pathArg := request.GetArguments()["path"] + pathArg := args["path"] if pathArg == nil { return nil, fmt.Errorf("argument 'path' is missing for tool %s", toolDef.Name) } @@ -507,6 +507,8 @@ func (m *McpLLMBinding) snykSendFeedback(invocationCtx workflow.InvocationContex return nil, fmt.Errorf("empty path given to tool %s", toolDef.Name) } + preventedIDs := coerceStringSlice(args["preventedIssueIds"]) + if preventedCount == 0 && remediatedCount == 0 { return mcp.NewToolResultText("No issues to send feedback for"), nil } @@ -515,17 +517,49 @@ func (m *McpLLMBinding) snykSendFeedback(invocationCtx workflow.InvocationContex m.updateGafConfigWithIntegrationEnvironment(invocationCtx, clientInfo.Name, clientInfo.Version) event := analytics.NewAnalyticsEventParam("Send feedback", nil, types.FilePath(path)) - - event.Extension = map[string]any{ - "mcp::preventedIssuesCount": int(preventedCount), - "mcp::remediatedIssuesCount": int(remediatedCount), - } + event.Extension = buildSendFeedbackExtension(&logger, int(preventedCount), int(remediatedCount), preventedIDs) go analytics.SendAnalytics(invocationCtx.GetEngine(), "", event, nil) return mcp.NewToolResultText("Successfully sent feedback"), nil } } +// coerceStringSlice converts a JSON-decoded array argument (`[]any`) into `[]string`, +// dropping any element that isn't a string. Returns nil when the input isn't a slice. +func coerceStringSlice(v any) []string { + raw, ok := v.([]any) + if !ok { + return nil + } + out := make([]string, 0, len(raw)) + for _, item := range raw { + if s, ok := item.(string); ok { + out = append(out, s) + } + } + return out +} + +// buildSendFeedbackExtension builds the analytics event Extension map for snyk_send_feedback. +// Logs a warning when the supplied preventedIssueIds length disagrees with preventedCount; +// the count remains authoritative for the metric. +func buildSendFeedbackExtension(logger *zerolog.Logger, preventedCount, remediatedCount int, preventedIDs []string) map[string]any { + if logger != nil && len(preventedIDs) > 0 && len(preventedIDs) != preventedCount { + logger.Warn(). + Int("preventedIssuesCount", preventedCount). + Int("preventedIssueIdsLen", len(preventedIDs)). + Msg("preventedIssueIds length does not match preventedIssuesCount; count is authoritative") + } + ext := map[string]any{ + "mcp::preventedIssuesCount": preventedCount, + "mcp::remediatedIssuesCount": remediatedCount, + } + if len(preventedIDs) > 0 { + ext["mcp::preventedIssueIds"] = preventedIDs + } + return ext +} + func (m *McpLLMBinding) snykTrustHandler(invocationCtx workflow.InvocationContext, toolDef SnykMcpToolsDefinition) func(ctx context.Context, request mcp.CallToolRequest) (*mcp.CallToolResult, error) { return func(ctx context.Context, request mcp.CallToolRequest) (*mcp.CallToolResult, error) { logger := m.logger.With().Str("method", toolDef.Name).Logger() diff --git a/internal/mcp/tools_test.go b/internal/mcp/tools_test.go index 23e207c..5efe4d7 100644 --- a/internal/mcp/tools_test.go +++ b/internal/mcp/tools_test.go @@ -17,6 +17,7 @@ package mcp import ( + "bytes" "context" "encoding/json" "fmt" @@ -1731,6 +1732,134 @@ func TestSnykTrustHandler(t *testing.T) { }) } +func TestSnykSendFeedbackHandler_Validation(t *testing.T) { + fixture := setupTestFixture(t) + toolDef := getToolWithName(t, fixture.tools, ToolName.SendFeedback) + require.NotNil(t, toolDef, "snyk_send_feedback tool definition not found") + + handler := fixture.binding.snykSendFeedback(fixture.invocationContext, *toolDef) + + t.Run("MissingPreventedIssuesCount", func(t *testing.T) { + req := mcp.CallToolRequest{Params: mcp.CallToolParams{Arguments: map[string]interface{}{ + "fixedExistingIssuesCount": float64(0), + "path": "/tmp", + }}} + result, err := handler(t.Context(), req) + require.Error(t, err) + require.Nil(t, result) + require.Contains(t, err.Error(), "preventedIssuesCount") + }) + + t.Run("MissingFixedExistingIssuesCount", func(t *testing.T) { + req := mcp.CallToolRequest{Params: mcp.CallToolParams{Arguments: map[string]interface{}{ + "preventedIssuesCount": float64(1), + "path": "/tmp", + }}} + result, err := handler(t.Context(), req) + require.Error(t, err) + require.Nil(t, result) + require.Contains(t, err.Error(), "fixedExistingIssuesCount") + }) + + t.Run("MissingPath", func(t *testing.T) { + req := mcp.CallToolRequest{Params: mcp.CallToolParams{Arguments: map[string]interface{}{ + "preventedIssuesCount": float64(1), + "fixedExistingIssuesCount": float64(0), + }}} + result, err := handler(t.Context(), req) + require.Error(t, err) + require.Nil(t, result) + require.Contains(t, err.Error(), "'path' is missing") + }) + + t.Run("EmptyPath", func(t *testing.T) { + req := mcp.CallToolRequest{Params: mcp.CallToolParams{Arguments: map[string]interface{}{ + "preventedIssuesCount": float64(1), + "fixedExistingIssuesCount": float64(0), + "path": "", + }}} + result, err := handler(t.Context(), req) + require.Error(t, err) + require.Nil(t, result) + require.Contains(t, err.Error(), "empty path") + }) + + t.Run("BothCountsZeroReturnsEarly", func(t *testing.T) { + // Both counts zero short-circuits before any analytics dispatch, so this + // is the success path safe to exercise without mocking the analytics workflow. + req := mcp.CallToolRequest{Params: mcp.CallToolParams{Arguments: map[string]interface{}{ + "preventedIssuesCount": float64(0), + "fixedExistingIssuesCount": float64(0), + "path": "/tmp", + "preventedIssueIds": []any{"sast:ignored"}, + }}} + result, err := handler(t.Context(), req) + require.NoError(t, err) + require.NotNil(t, result) + }) +} + +func TestCoerceStringSlice(t *testing.T) { + t.Run("NilInput", func(t *testing.T) { + require.Nil(t, coerceStringSlice(nil)) + }) + + t.Run("NonSliceInput", func(t *testing.T) { + require.Nil(t, coerceStringSlice("not a slice")) + require.Nil(t, coerceStringSlice(42)) + }) + + t.Run("EmptySlice", func(t *testing.T) { + got := coerceStringSlice([]any{}) + require.NotNil(t, got) + require.Len(t, got, 0) + }) + + t.Run("AllStrings", func(t *testing.T) { + got := coerceStringSlice([]any{"a", "b", "c"}) + require.Equal(t, []string{"a", "b", "c"}, got) + }) + + t.Run("MixedTypesDropsNonStrings", func(t *testing.T) { + got := coerceStringSlice([]any{"a", 1, "b", true, "c"}) + require.Equal(t, []string{"a", "b", "c"}, got) + }) +} + +func TestBuildSendFeedbackExtension(t *testing.T) { + t.Run("CountsOnlyNoIDs", func(t *testing.T) { + ext := buildSendFeedbackExtension(nil, 2, 1, nil) + require.Equal(t, 2, ext["mcp::preventedIssuesCount"]) + require.Equal(t, 1, ext["mcp::remediatedIssuesCount"]) + _, hasIDs := ext["mcp::preventedIssueIds"] + require.False(t, hasIDs, "preventedIssueIds key must be omitted when no IDs provided") + }) + + t.Run("WithMatchingIDs", func(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) + require.Equal(t, ids, ext["mcp::preventedIssueIds"]) + require.Empty(t, buf.String(), "no warning expected when length matches count") + }) + + t.Run("CountMismatchLogsWarningButIncludesIDs", func(t *testing.T) { + ids := []string{"sast:a", "sast:b"} + var buf bytes.Buffer + logger := zerolog.New(&buf) + ext := buildSendFeedbackExtension(&logger, 3, 0, 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{}) + _, hasIDs := ext["mcp::preventedIssueIds"] + require.False(t, hasIDs) + }) +} + func TestHandleFileOutput(t *testing.T) { logger := zerolog.New(io.Discard) testOutput := `{"test": "output", "issues": [{"id": "1", "severity": "high"}]}` diff --git a/internal/mcp/utils.go b/internal/mcp/utils.go index 6557046..3b9a57d 100644 --- a/internal/mcp/utils.go +++ b/internal/mcp/utils.go @@ -107,6 +107,15 @@ func createToolFromDefinition(toolDef *SnykMcpToolsDefinition) mcp.Tool { } else { opts = append(opts, mcp.WithNumber(param.Name, mcp.Description(param.Description))) } + 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...)) } }