fix(mcp): never marshal nil citations slice as JSON null

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

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

pkg/mcp/git/tools_draft_pr.go

@@ -76,8 +76,9 @@ var prBodyMarkerPattern = regexp.MustCompile(`(?s)<<<PR_BODY\s*\n(.*?)\nPR_BODY>
 
 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)

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/<name> 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,15 +123,15 @@ 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 == "" {
 		refRange = "(commits within the last 30 days — use `git log --since='30 days'`)"
 	}
 	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.

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 <<<CITATIONS>>> 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)
+}

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,

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,

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