From d9ce6e9e03f1a8874938e7361c5d23bdf1aa7f28 Mon Sep 17 00:00:00 2001 From: Victor Ma Date: Fri, 10 Jul 2026 10:00:35 -0400 Subject: [PATCH 1/3] 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 42d056f..1f55ffc 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 9fdf591..5067a54 100644 --- a/internal/mcp/tools_test.go +++ b/internal/mcp/tools_test.go @@ -2516,9 +2516,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 90e230f91de82a170b1c83abfecf03a0d1cafaed Mon Sep 17 00:00:00 2001 From: Victor Ma Date: Fri, 10 Jul 2026 10:07:37 -0400 Subject: [PATCH 2/3] 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. --- Makefile | 2 +- internal/mcp/snyk_tools.json | 77 +++++++++++++++- internal/mcp/tools.go | 135 +++++++++++++++++++++++++--- internal/mcp/tools_test.go | 166 ++++++++++++++++++++++++++++++++++- 4 files changed, 362 insertions(+), 18 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/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 1f55ffc..b69c19c 100644 --- a/internal/mcp/tools.go +++ b/internal/mcp/tools.go @@ -510,6 +510,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 @@ -519,7 +520,20 @@ func (m *McpLLMBinding) snykSendFeedback(invocationCtx workflow.InvocationContex m.updateGafConfigWithIntegrationEnvironment(invocationCtx, clientInfo.Name, clientInfo.Version) 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, + fixedIssuesBySeverity: coerceBreakdown(args["fixedIssuesBySeverity"], severityBreakdownKeys), + preventedIssuesBySeverity: coerceBreakdown(args["preventedIssuesBySeverity"], severityBreakdownKeys), + fixedIssuesByScanType: coerceBreakdown(args["fixedIssuesByScanType"], scanTypeBreakdownKeys), + outcome: mcp.ExtractString(args, "outcome"), + breakabilityRisk: mcp.ExtractString(args, "breakabilityRisk"), + breakabilityRiskSource: mcp.ExtractString(args, "breakabilityRiskSource"), + strategy: mcp.ExtractString(args, "strategy"), + testsPassed: coerceOptionalBoolPtr(args["testsPassed"]), + }) go analytics.SendAnalytics(invocationCtx.GetEngine(), "", event, nil) return mcp.NewToolResultText("Successfully sent feedback"), nil @@ -542,22 +556,119 @@ func coerceStringSlice(v any) []string { 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 { +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 +} + +// 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 + fixedIssuesBySeverity map[string]int + preventedIssuesBySeverity map[string]int + fixedIssuesByScanType map[string]int + outcome string + breakabilityRisk string + breakabilityRiskSource string + strategy string + testsPassed *bool +} + +// 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(). - 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(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 len(preventedIDs) > 0 { - ext["mcp::preventedIssueIds"] = preventedIDs + if p.testsPassed != nil { + ext["mcp::testsPassed"] = *p.testsPassed } return ext } diff --git a/internal/mcp/tools_test.go b/internal/mcp/tools_test.go index 5067a54..edacfb6 100644 --- a/internal/mcp/tools_test.go +++ b/internal/mcp/tools_test.go @@ -1890,6 +1890,71 @@ func TestSnykSendFeedbackHandler_CorrelationID(t *testing.T) { require.Contains(t, string(payloads[1]), expectedURN) } +// TestSnykSendFeedbackHandler_FixedIssueIds confirms fixedIssueIds is parsed +// from the actual MCP request args map and forwarded onto the analytics +// event, mirroring the existing preventedIssueIds handling. +func TestSnykSendFeedbackHandler_FixedIssueIds(t *testing.T) { + fixture := setupTestFixture(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/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::fixedIssueIds":["sast:javascript/SqlInjection"]`) +} + +// TestSnykSendFeedbackHandler_PreventedIssuesBySeverityLiteralKey guards +// 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. +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)) @@ -1919,7 +1984,7 @@ func TestCoerceStringSlice(t *testing.T) { func TestBuildSendFeedbackExtension(t *testing.T) { t.Run("CountsOnlyNoIDs", func(t *testing.T) { - ext := buildSendFeedbackExtension(nil, 2, 1, nil) + ext := buildSendFeedbackExtension(nil, sendFeedbackParams{preventedCount: 2, remediatedCount: 1}) require.Equal(t, 2, ext["mcp::preventedIssuesCount"]) require.Equal(t, 1, ext["mcp::remediatedIssuesCount"]) _, hasIDs := ext["mcp::preventedIssueIds"] @@ -1930,7 +1995,7 @@ func TestBuildSendFeedbackExtension(t *testing.T) { ids := []string{"sast:javascript/SqlInjection", "sca:SNYK-JS-LODASH-1234567"} var buf bytes.Buffer logger := zerolog.New(&buf) - ext := buildSendFeedbackExtension(&logger, 2, 0, ids) + ext := buildSendFeedbackExtension(&logger, sendFeedbackParams{preventedCount: 2, preventedIDs: ids}) require.Equal(t, ids, ext["mcp::preventedIssueIds"]) require.Empty(t, buf.String(), "no warning expected when length matches count") }) @@ -1939,16 +2004,109 @@ 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("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", + } { + _, 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, + }) + 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"]) + }) + + 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 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 37b7d016a64c72705a3cecb54bb29c7e0563e5ec Mon Sep 17 00:00:00 2001 From: Victor Ma Date: Thu, 16 Jul 2026 16:17:48 -0400 Subject: [PATCH 3/3] refactor: address review feedback on richer feedback metrics Warn (rather than silently drop) non-numeric breakdown values, reuse mcp.ExtractMap instead of re-implementing its type assertion, wrap the outcome/breakabilityRisk/breakabilityRiskSource/strategy fields in distinct types so a copy-paste swap fails to compile, and table-drive the optional-field assembly in buildSendFeedbackExtension. --- internal/mcp/tools.go | 109 +++++++++++++++++++++---------------- internal/mcp/tools_test.go | 18 +++--- 2 files changed, 73 insertions(+), 54 deletions(-) diff --git a/internal/mcp/tools.go b/internal/mcp/tools.go index b69c19c..7715d8f 100644 --- a/internal/mcp/tools.go +++ b/internal/mcp/tools.go @@ -525,13 +525,13 @@ func (m *McpLLMBinding) snykSendFeedback(invocationCtx workflow.InvocationContex remediatedCount: int(remediatedCount), preventedIDs: preventedIDs, fixedIDs: fixedIDs, - fixedIssuesBySeverity: coerceBreakdown(args["fixedIssuesBySeverity"], severityBreakdownKeys), - preventedIssuesBySeverity: coerceBreakdown(args["preventedIssuesBySeverity"], severityBreakdownKeys), - fixedIssuesByScanType: coerceBreakdown(args["fixedIssuesByScanType"], scanTypeBreakdownKeys), - outcome: mcp.ExtractString(args, "outcome"), - breakabilityRisk: mcp.ExtractString(args, "breakabilityRisk"), - breakabilityRiskSource: mcp.ExtractString(args, "breakabilityRiskSource"), - strategy: mcp.ExtractString(args, "strategy"), + fixedIssuesBySeverity: coerceBreakdown(&logger, args, "fixedIssuesBySeverity", severityBreakdownKeys), + preventedIssuesBySeverity: coerceBreakdown(&logger, args, "preventedIssuesBySeverity", severityBreakdownKeys), + fixedIssuesByScanType: coerceBreakdown(&logger, args, "fixedIssuesByScanType", scanTypeBreakdownKeys), + outcome: remediationOutcome(mcp.ExtractString(args, "outcome")), + breakabilityRisk: breakabilityRiskLevel(mcp.ExtractString(args, "breakabilityRisk")), + breakabilityRiskSource: breakabilityRiskSourceKind(mcp.ExtractString(args, "breakabilityRiskSource")), + strategy: remediationStrategy(mcp.ExtractString(args, "strategy")), testsPassed: coerceOptionalBoolPtr(args["testsPassed"]), }) go analytics.SendAnalytics(invocationCtx.GetEngine(), "", event, nil) @@ -561,15 +561,16 @@ 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 +// coerceBreakdown extracts the argKey field of args and converts it into a +// `map[string]int`, keeping only the supplied keys and only values that +// decode as numbers. +// Returns nil when the field 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 { +// sending an empty map. Logs a warning (rather than silently dropping) when a +// recognized key is present with a non-numeric value. +func coerceBreakdown(logger *zerolog.Logger, args map[string]any, argKey string, keys []string) map[string]int { + raw := mcp.ExtractMap(args, argKey) + if raw == nil { return nil } out := make(map[string]int) @@ -578,9 +579,17 @@ func coerceBreakdown(v any, keys []string) map[string]int { if !exists { continue } - if f, ok := val.(float64); ok { - out[key] = int(f) + f, ok := val.(float64) + if !ok { + if logger != nil { + logger.Warn(). + Str("field", argKey). + Str("key", key). + Msg("breakdown value is not a number; dropping") + } + continue } + out[key] = int(f) } if len(out) == 0 { return nil @@ -599,6 +608,16 @@ func coerceOptionalBoolPtr(v any) *bool { return &b } +// remediationOutcome, breakabilityRiskLevel, breakabilityRiskSourceKind, and +// remediationStrategy are distinct string types (not bare strings) for the +// enum-shaped sendFeedbackParams fields below, so a copy-paste swap between +// two of these fields fails to compile instead of silently mislabeling the +// analytics event. +type remediationOutcome string +type breakabilityRiskLevel string +type breakabilityRiskSourceKind string +type remediationStrategy string + // sendFeedbackParams holds every optional/required argument parsed from a // snyk_send_feedback call, used to build the analytics event Extension map. type sendFeedbackParams struct { @@ -609,10 +628,10 @@ type sendFeedbackParams struct { fixedIssuesBySeverity map[string]int preventedIssuesBySeverity map[string]int fixedIssuesByScanType map[string]int - outcome string - breakabilityRisk string - breakabilityRiskSource string - strategy string + outcome remediationOutcome + breakabilityRisk breakabilityRiskLevel + breakabilityRiskSource breakabilityRiskSourceKind + strategy remediationStrategy testsPassed *bool } @@ -640,32 +659,28 @@ func buildSendFeedbackExtension(logger *zerolog.Logger, p sendFeedbackParams) ma "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(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 + + // Each optional richness field is added only when present, so callers can + // omit fields they haven't computed rather than sending zero values. + optionalFields := []struct { + key string + value any + present bool + }{ + {"mcp::preventedIssueIds", p.preventedIDs, len(p.preventedIDs) > 0}, + {"mcp::fixedIssueIds", p.fixedIDs, len(p.fixedIDs) > 0}, + {"mcp::fixedIssuesBySeverity", p.fixedIssuesBySeverity, len(p.fixedIssuesBySeverity) > 0}, + {"mcp::preventedIssuesBySeverity", p.preventedIssuesBySeverity, len(p.preventedIssuesBySeverity) > 0}, + {"mcp::fixedIssuesByScanType", p.fixedIssuesByScanType, len(p.fixedIssuesByScanType) > 0}, + {"mcp::outcome", string(p.outcome), p.outcome != ""}, + {"mcp::breakabilityRisk", string(p.breakabilityRisk), p.breakabilityRisk != ""}, + {"mcp::breakabilityRiskSource", string(p.breakabilityRiskSource), p.breakabilityRiskSource != ""}, + {"mcp::strategy", string(p.strategy), p.strategy != ""}, + } + for _, f := range optionalFields { + if f.present { + ext[f.key] = f.value + } } if p.testsPassed != nil { ext["mcp::testsPassed"] = *p.testsPassed diff --git a/internal/mcp/tools_test.go b/internal/mcp/tools_test.go index edacfb6..971beec 100644 --- a/internal/mcp/tools_test.go +++ b/internal/mcp/tools_test.go @@ -2079,25 +2079,29 @@ func TestBuildSendFeedbackExtension(t *testing.T) { 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)) + require.Nil(t, coerceBreakdown(nil, map[string]any{"field": "not an object"}, "field", severityBreakdownKeys)) + require.Nil(t, coerceBreakdown(nil, map[string]any{"field": nil}, "field", severityBreakdownKeys)) + require.Nil(t, coerceBreakdown(nil, map[string]any{}, "field", severityBreakdownKeys)) }) t.Run("EmptyObject", func(t *testing.T) { - require.Nil(t, coerceBreakdown(map[string]any{}, severityBreakdownKeys)) + require.Nil(t, coerceBreakdown(nil, map[string]any{"field": map[string]any{}}, "field", severityBreakdownKeys)) }) t.Run("OnlyRecognizedKeysKept", func(t *testing.T) { - got := coerceBreakdown(map[string]any{ + got := coerceBreakdown(nil, map[string]any{"field": map[string]any{ "critical": float64(1), "unknown": float64(9), - }, severityBreakdownKeys) + }}, "field", 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) + t.Run("NonNumericValueDroppedAndWarned", func(t *testing.T) { + var buf bytes.Buffer + logger := zerolog.New(&buf) + got := coerceBreakdown(&logger, map[string]any{"field": map[string]any{"sast": "not a number", "sca": float64(2)}}, "field", scanTypeBreakdownKeys) require.Equal(t, map[string]int{"sca": 2}, got) + require.Contains(t, buf.String(), "not a number") }) }