feat: [ADS-394] Log Findings

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:<ruleId>' (e.g. 'sast:javascript/SqlInjection') or 'sca:<snykId>' (e.g. 'sca:SNYK-JS-LODASH-1234567'). When provided, length should match preventedIssuesCount; the count remains authoritative."
         }
       ]
     },

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()

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"}]}`

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...))
 		}
 	}