From 5b31f49ed5e8ece003d26df86d4c7e95087c35ba Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= <54936225+sourcehawk@users.noreply.github.com> Date: Tue, 9 Jun 2026 18:07:24 +0200 Subject: [PATCH] fix(mcp): never marshal nil citations slice as JSON null MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Citation-bearing tool outputs (analyze_change, correlate_with_findings, draft_pr, summarize_thread, analyze_channel) declare a non-nullable `citations` array in their auto-generated MCP output schema. A nil Go slice marshals to JSON `null`, which the go-sdk's own output validation rejects with `validating tool output: ... has type "null", want "array"`, masking the real result with an opaque error. Two nil sources: citations.Run left Citations nil on its soft-fail returns, and the handlers' early-return paths built output structs with a nil slice. The SDK validates the typed Out whenever the handler's Go error return is nil, so every return path was affected — and since these handlers report failures via errorResult (nil Go error), the early returns were validated too. Enforce one invariant: a citation-bearing Out never marshals a nil slice. Fix the shared citations.Run soft-fail paths plus the per-handler early-return literals. Add in-memory wire tests that exercise the real SDK output validation (handler unit tests bypass it); they reproduce the exact error pre-fix. Co-Authored-By: Claude Opus 4.8 (1M context) --- pkg/mcp/citations/runner.go | 2 + pkg/mcp/citations/runner_test.go | 2 + pkg/mcp/git/tools_draft_pr.go | 5 +- pkg/mcp/git/tools_subagent.go | 10 +- pkg/mcp/git/tools_subagent_wire_test.go | 112 ++++++++++++++++++++++ pkg/mcp/slack/tool_analyze.go | 9 +- pkg/mcp/slack/tool_summarize.go | 11 ++- pkg/mcp/slack/tools_subagent_wire_test.go | 59 ++++++++++++ 8 files changed, 194 insertions(+), 16 deletions(-) create mode 100644 pkg/mcp/git/tools_subagent_wire_test.go create mode 100644 pkg/mcp/slack/tools_subagent_wire_test.go diff --git a/pkg/mcp/citations/runner.go b/pkg/mcp/citations/runner.go index 3a927c1..a99c05d 100644 --- a/pkg/mcp/citations/runner.go +++ b/pkg/mcp/citations/runner.go @@ -68,6 +68,7 @@ func Run(ctx context.Context, in RunInput) (Outcome, error) { if err != nil { return Outcome{ Prose: prose1, + Citations: []Citation{}, CitationsParseError: combine(errs1) + " (retry transport error: " + err.Error() + ")", TimedOut: raw1.TimedOut, }, nil @@ -84,6 +85,7 @@ func Run(ctx context.Context, in RunInput) (Outcome, error) { } return Outcome{ Prose: finalProse, + Citations: []Citation{}, CitationsParseError: combine(errs2), TimedOut: raw2.TimedOut, }, nil diff --git a/pkg/mcp/citations/runner_test.go b/pkg/mcp/citations/runner_test.go index c42341d..644d1b1 100644 --- a/pkg/mcp/citations/runner_test.go +++ b/pkg/mcp/citations/runner_test.go @@ -86,6 +86,7 @@ CITATIONS>>>`}, nil out, err := Run(context.Background(), RunInput{Run: run, Validator: v, Prompt: "ask"}) require.NoError(t, err, "soft-fail should not propagate as Go error") assert.Equal(t, 2, calls, "retry budget capped at 1 — total calls is 2") + assert.NotNil(t, out.Citations, "soft-fail Citations must be a non-nil empty slice so it marshals as [] not null — a nil slice trips the consumer's non-nullable array output schema") assert.Empty(t, out.Citations) assert.NotEmpty(t, out.CitationsParseError) assert.Contains(t, out.Prose, "claim [1]", "prose still surfaced on soft-fail") @@ -156,6 +157,7 @@ CITATIONS>>>`}, nil out, err := Run(context.Background(), RunInput{Run: run, Validator: v, Prompt: "ask"}) require.NoError(t, err, "retry transport failure should soft-fail, not error") assert.Equal(t, 2, calls) + assert.NotNil(t, out.Citations, "retry-transport soft-fail Citations must be non-nil so it marshals as [] not null") assert.Contains(t, out.CitationsParseError, "retry transport") assert.Contains(t, out.Prose, "claim [1]") } diff --git a/pkg/mcp/git/tools_draft_pr.go b/pkg/mcp/git/tools_draft_pr.go index 9305048..b711e5f 100644 --- a/pkg/mcp/git/tools_draft_pr.go +++ b/pkg/mcp/git/tools_draft_pr.go @@ -76,8 +76,9 @@ var prBodyMarkerPattern = regexp.MustCompile(`(?s)<< func (s *Server) draftPR(ctx context.Context, _ *mcp.CallToolRequest, in draftPRIn) (*mcp.CallToolResult, draftPROut, error) { out := draftPROut{ - Repo: s.repoFull(), - IssueURL: strings.TrimSpace(in.IssueURL), + Repo: s.repoFull(), + IssueURL: strings.TrimSpace(in.IssueURL), + Citations: []citations.Citation{}, } issueOwner, issueRepo, issueNum, ok := parseThisRepoIssueURL(in.IssueURL) diff --git a/pkg/mcp/git/tools_subagent.go b/pkg/mcp/git/tools_subagent.go index c5b57f6..e7cb931 100644 --- a/pkg/mcp/git/tools_subagent.go +++ b/pkg/mcp/git/tools_subagent.go @@ -44,17 +44,17 @@ type correlateOut struct { func (s *Server) analyzeChange(ctx context.Context, _ *mcp.CallToolRequest, in analyzeChangeIn) (*mcp.CallToolResult, analyzeChangeOut, error) { if in.Ref == "" || in.Question == "" { - return errorResult("ref and question are required"), analyzeChangeOut{Repo: s.repoFull(), Ref: in.Ref}, nil + return errorResult("ref and question are required"), analyzeChangeOut{Repo: s.repoFull(), Ref: in.Ref, Citations: []citations.Citation{}}, nil } dir, err := s.EnsureClone(ctx) if err != nil { - return errorResult(err.Error()), analyzeChangeOut{Repo: s.repoFull(), Ref: in.Ref}, nil + return errorResult(err.Error()), analyzeChangeOut{Repo: s.repoFull(), Ref: in.Ref, Citations: []citations.Citation{}}, nil } // Rewrite bare branch names to origin/ so the sub-agent's // git invocations operate on the fresh tip, not the stale local ref. resolvedRef, err := s.resolveRef(ctx, dir, in.Ref) if err != nil { - return errorResult(err.Error()), analyzeChangeOut{Repo: s.repoFull(), Ref: in.Ref}, nil + return errorResult(err.Error()), analyzeChangeOut{Repo: s.repoFull(), Ref: in.Ref, Citations: []citations.Citation{}}, nil } prompt := fmt.Sprintf( `You are analysing a single git change in a repository for an incident investigation. @@ -123,7 +123,7 @@ Self-verify before emitting the citations block: func (s *Server) correlateWithFindings(ctx context.Context, _ *mcp.CallToolRequest, in correlateIn) (*mcp.CallToolResult, correlateOut, error) { if in.Findings == "" { - return errorResult("findings is required"), correlateOut{Repo: s.repoFull()}, nil + return errorResult("findings is required"), correlateOut{Repo: s.repoFull(), Citations: []citations.Citation{}}, nil } refRange := in.RefRange if refRange == "" { @@ -131,7 +131,7 @@ func (s *Server) correlateWithFindings(ctx context.Context, _ *mcp.CallToolReque } dir, err := s.EnsureClone(ctx) if err != nil { - return errorResult(err.Error()), correlateOut{Repo: s.repoFull()}, nil + return errorResult(err.Error()), correlateOut{Repo: s.repoFull(), Citations: []citations.Citation{}}, nil } prompt := fmt.Sprintf( `You are correlating an incident's findings against recent code changes. diff --git a/pkg/mcp/git/tools_subagent_wire_test.go b/pkg/mcp/git/tools_subagent_wire_test.go new file mode 100644 index 0000000..6c37776 --- /dev/null +++ b/pkg/mcp/git/tools_subagent_wire_test.go @@ -0,0 +1,112 @@ +package git + +import ( + "context" + "path/filepath" + "testing" + + sdkmcp "github.com/modelcontextprotocol/go-sdk/mcp" + "github.com/sourcehawk/triagent/pkg/mcp/subagent" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// wireSession connects an in-memory client to the server's MCP impl so a +// tool call goes through the go-sdk's output-schema validation — the layer +// the handler unit tests bypass by calling the method directly. +func wireSession(t *testing.T, s *Server) *sdkmcp.ClientSession { + t.Helper() + s.impl = sdkmcp.NewServer(&sdkmcp.Implementation{Name: "wire-test", Version: "v0"}, nil) + s.register() + + srvT, cliT := sdkmcp.NewInMemoryTransports() + srvSess, err := s.impl.Connect(context.Background(), srvT, nil) + require.NoError(t, err) + t.Cleanup(func() { _ = srvSess.Close() }) + + client := sdkmcp.NewClient(&sdkmcp.Implementation{Name: "wire-test", Version: "v0"}, nil) + cliSess, err := client.Connect(context.Background(), cliT, nil) + require.NoError(t, err) + t.Cleanup(func() { _ = cliSess.Close() }) + return cliSess +} + +// TestWire_AnalyzeChange_CitationsSoftFail_ReturnsEmptyArrayNotNull is the +// faithful reproduction of the reported bug: a sub-agent that emits no +// citations block soft-fails, leaving Citations a nil slice. Marshaled as +// "citations":null it trips the auto-generated output schema (citations is +// a non-nullable array), so the SDK fails the whole call with "validating +// tool output" instead of returning the sub-agent's prose. The tool must +// instead surface the prose with citations as an empty array. +func TestWire_AnalyzeChange_CitationsSoftFail_ReturnsEmptyArrayNotNull(t *testing.T) { + t.Parallel() + cacheDir, _ := initFixtureRepo(t, "o", "n") + bareDir := filepath.Join(t.TempDir(), "origin.git") + runFixtureGit(t, "", "init", "--bare", bareDir) + runFixtureGit(t, filepath.Join(cacheDir, "o", "n"), "remote", "add", "origin", bareDir) + runFixtureGit(t, filepath.Join(cacheDir, "o", "n"), "push", "origin", "main") + runFixtureGit(t, filepath.Join(cacheDir, "o", "n"), "fetch", "--prune", "--tags") + + s := &Server{ + owner: "o", name: "n", cacheDir: cacheDir, gh: &stubGh{response: []byte("main\n")}, + runSubAgent: func(_ context.Context, _, _, _, _ string) (subagent.Result, error) { + // No <<>> block on either call → citations runner + // soft-fails after the corrective retry. + return subagent.Result{Summary: "the change is unrelated."}, nil + }, + } + cliSess := wireSession(t, s) + + res, err := cliSess.CallTool(context.Background(), &sdkmcp.CallToolParams{ + Name: "analyze_change", + Arguments: map[string]any{"ref": "v0.2.0", "question": "what changed?"}, + }) + require.NoError(t, err, "soft-failed citations must not trip output-schema validation") + require.False(t, res.IsError, "soft-fail should surface prose, not a tool error") + + structured, ok := res.StructuredContent.(map[string]any) + require.True(t, ok, "structured content should decode to an object") + cits, present := structured["citations"] + require.True(t, present, "citations key must be present") + require.NotNil(t, cits, "citations must be [] not null on soft-fail") + assert.Empty(t, cits, "soft-fail yields zero citations") +} + +// TestWire_AnalyzeChange_MissingArgs_NilCitationsDoesNotBreakSchema covers +// the handler's early-return path: with empty arguments it returns before +// any clone, so no fixture is needed. The returned output struct still has +// a nil Citations slice, and — because the handler reports failures via the +// result (not the Go error return) — the SDK marshals and validates it. +// Pre-fix that validation fails on "citations":null and the handler's +// "ref and question are required" message is lost. +func TestWire_AnalyzeChange_MissingArgs_NilCitationsDoesNotBreakSchema(t *testing.T) { + t.Parallel() + s := &Server{owner: "o", name: "n", gh: &stubGh{response: []byte("main\n")}} + cliSess := wireSession(t, s) + + res, err := cliSess.CallTool(context.Background(), &sdkmcp.CallToolParams{ + Name: "analyze_change", + Arguments: map[string]any{"ref": "", "question": ""}, + }) + require.NoError(t, err, "nil Citations on the early-return path must not trip output-schema validation") + require.True(t, res.IsError, "missing args must surface as a tool error") + require.NotEmpty(t, res.Content) +} + +// TestWire_DraftPR_MissingIssueURL_NilCitationsDoesNotBreakSchema covers +// draft_pr's early-return path (its draftPROut also carries a non-nullable +// citations array). An invalid issue_url returns before the worktree is +// built, with a nil Citations slice pre-fix. +func TestWire_DraftPR_MissingIssueURL_NilCitationsDoesNotBreakSchema(t *testing.T) { + t.Parallel() + s := &Server{owner: "o", name: "n", gh: &stubGh{response: []byte("main\n")}} + cliSess := wireSession(t, s) + + res, err := cliSess.CallTool(context.Background(), &sdkmcp.CallToolParams{ + Name: "draft_pr", + Arguments: map[string]any{"issue_url": ""}, + }) + require.NoError(t, err, "nil Citations on draft_pr's early return must not trip output-schema validation") + require.True(t, res.IsError, "missing issue_url must surface as a tool error") + require.NotEmpty(t, res.Content) +} diff --git a/pkg/mcp/slack/tool_analyze.go b/pkg/mcp/slack/tool_analyze.go index c7a4554..a31f776 100644 --- a/pkg/mcp/slack/tool_analyze.go +++ b/pkg/mcp/slack/tool_analyze.go @@ -32,19 +32,19 @@ type analyzeChannelOut struct { func (s *Server) handleAnalyzeChannel(ctx context.Context, _ *mcp.CallToolRequest, in analyzeChannelIn) (*mcp.CallToolResult, analyzeChannelOut, error) { channelID := strings.TrimSpace(in.ChannelID) if channelID == "" { - return errorResult("channel_id is required"), analyzeChannelOut{}, nil + return errorResult("channel_id is required"), analyzeChannelOut{Citations: []citations.Citation{}}, nil } if in.DesiredFindings == "" { - return errorResult("desired_findings is required"), analyzeChannelOut{}, nil + return errorResult("desired_findings is required"), analyzeChannelOut{Citations: []citations.Citation{}}, nil } store, err := s.resolveStore(channelID) if err != nil { - return errorResult(err.Error()), analyzeChannelOut{}, nil + return errorResult(err.Error()), analyzeChannelOut{Citations: []citations.Citation{}}, nil } syncRes, err := store.Sync(ctx, syncFull, in.SinceUnix) if err != nil { - return errorResult(err.Error()), analyzeChannelOut{}, nil + return errorResult(err.Error()), analyzeChannelOut{Citations: []citations.Citation{}}, nil } prompt := fmt.Sprintf(`You are analysing an entire Slack channel. This message is your COMPLETE task — there is no prior conversation, no missing context, no original question to ask about. Everything you need is below: the working directory holds the cached channel content, and your findings request is stated explicitly. @@ -75,6 +75,7 @@ Self-verify before emitting the citations block: for each candidate thread_ts, G res, runErr := s.runSubAgentWithCitations(ctx, prompt, parentID, channelID, store) if runErr != nil { return errorResult(runErr.Error()), analyzeChannelOut{ + Citations: []citations.Citation{}, PromptSent: prompt, ParentCount: syncRes.ParentCount, Truncated: syncRes.Truncated, diff --git a/pkg/mcp/slack/tool_summarize.go b/pkg/mcp/slack/tool_summarize.go index 3c96c78..d0db451 100644 --- a/pkg/mcp/slack/tool_summarize.go +++ b/pkg/mcp/slack/tool_summarize.go @@ -29,22 +29,22 @@ type summarizeThreadOut struct { func (s *Server) handleSummarizeThread(ctx context.Context, _ *mcp.CallToolRequest, in summarizeThreadIn) (*mcp.CallToolResult, summarizeThreadOut, error) { channelID := strings.TrimSpace(in.ChannelID) if channelID == "" { - return errorResult("channel_id is required"), summarizeThreadOut{}, nil + return errorResult("channel_id is required"), summarizeThreadOut{Citations: []citations.Citation{}}, nil } if in.ThreadTS == "" { - return errorResult("thread_ts is required"), summarizeThreadOut{}, nil + return errorResult("thread_ts is required"), summarizeThreadOut{Citations: []citations.Citation{}}, nil } if in.DesiredFindings == "" { - return errorResult("desired_findings is required"), summarizeThreadOut{}, nil + return errorResult("desired_findings is required"), summarizeThreadOut{Citations: []citations.Citation{}}, nil } store, err := s.resolveStore(channelID) if err != nil { - return errorResult(err.Error()), summarizeThreadOut{}, nil + return errorResult(err.Error()), summarizeThreadOut{Citations: []citations.Citation{}}, nil } _, rateLimited, err := store.SyncThread(ctx, in.ThreadTS) if err != nil { - return errorResult(err.Error()), summarizeThreadOut{}, nil + return errorResult(err.Error()), summarizeThreadOut{Citations: []citations.Citation{}}, nil } prompt := fmt.Sprintf(`You are analysing one Slack thread. This message is your COMPLETE task — there is no prior conversation, no missing context, no original question to ask about. Everything you need is below: the working directory holds the cached thread content, and your findings request is stated explicitly. @@ -76,6 +76,7 @@ Self-verify before emitting the citations block: this tool resolved one thread, res, runErr := s.runSubAgentWithCitations(ctx, prompt, parentID, channelID, store) if runErr != nil { return errorResult(runErr.Error()), summarizeThreadOut{ + Citations: []citations.Citation{}, ThreadTS: in.ThreadTS, PromptSent: prompt, RateLimited: rateLimited, diff --git a/pkg/mcp/slack/tools_subagent_wire_test.go b/pkg/mcp/slack/tools_subagent_wire_test.go new file mode 100644 index 0000000..c43c151 --- /dev/null +++ b/pkg/mcp/slack/tools_subagent_wire_test.go @@ -0,0 +1,59 @@ +package slack + +import ( + "context" + "testing" + + sdkmcp "github.com/modelcontextprotocol/go-sdk/mcp" + "github.com/stretchr/testify/require" +) + +// slackWireSession connects an in-memory client to the server's MCP impl so +// a tool call goes through the go-sdk's output-schema validation — the layer +// the handler unit tests bypass by calling the method directly. +func slackWireSession(t *testing.T) *sdkmcp.ClientSession { + t.Helper() + s, err := New(Options{Token: "xoxp-test", CacheDir: t.TempDir()}) + require.NoError(t, err) + + srvT, cliT := sdkmcp.NewInMemoryTransports() + srvSess, err := s.impl.Connect(context.Background(), srvT, nil) + require.NoError(t, err) + t.Cleanup(func() { _ = srvSess.Close() }) + + client := sdkmcp.NewClient(&sdkmcp.Implementation{Name: "wire-test", Version: "v0"}, nil) + cliSess, err := client.Connect(context.Background(), cliT, nil) + require.NoError(t, err) + t.Cleanup(func() { _ = cliSess.Close() }) + return cliSess +} + +// TestWire_Slack_CitationTools_MissingArgs_NilCitationsDoesNotBreakSchema +// covers the early-return paths of the two slack sub-agent tools, whose Out +// structs carry a non-nullable citations array. A missing required argument +// returns before any sub-agent run, with a nil Citations slice pre-fix — +// which the SDK rejects as "citations":null, replacing the handler's +// "… is required" message with an opaque output-schema error. +func TestWire_Slack_CitationTools_MissingArgs_NilCitationsDoesNotBreakSchema(t *testing.T) { + t.Parallel() + cliSess := slackWireSession(t) + + cases := []struct { + tool string + args map[string]any + }{ + {"summarize_thread", map[string]any{"channel_id": "", "thread_ts": "", "desired_findings": ""}}, + {"analyze_channel", map[string]any{"channel_id": "", "desired_findings": ""}}, + } + for _, tc := range cases { + t.Run(tc.tool, func(t *testing.T) { + res, err := cliSess.CallTool(context.Background(), &sdkmcp.CallToolParams{ + Name: tc.tool, + Arguments: tc.args, + }) + require.NoError(t, err, "nil Citations on the early-return path must not trip output-schema validation") + require.True(t, res.IsError, "missing args must surface as a tool error") + require.NotEmpty(t, res.Content) + }) + } +}