From a556440af59a1f06e1a442b23c9cdc8b2da9f0d2 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Wed, 26 Aug 2026 13:28:48 +0000 Subject: [PATCH 1/5] Continue after rejected tool calls Co-authored-by: Nick Misasi --- conversations/ask_user_question_flow_test.go | 77 ++-- conversations/tool_approval.go | 19 +- conversations/tool_approval_license_test.go | 12 +- conversations/tool_rejection_followup_test.go | 376 ++++++++++++++++++ llm/tool_retry.go | 51 ++- llm/tool_retry_test.go | 48 +++ 6 files changed, 529 insertions(+), 54 deletions(-) create mode 100644 conversations/tool_rejection_followup_test.go diff --git a/conversations/ask_user_question_flow_test.go b/conversations/ask_user_question_flow_test.go index 7e2c42bcd..e1c4ba006 100644 --- a/conversations/ask_user_question_flow_test.go +++ b/conversations/ask_user_question_flow_test.go @@ -401,15 +401,16 @@ func TestHandleToolCallAutoExecutesPolicyEligiblePendingTools(t *testing.T) { const origin = "https://jira.example.com" cases := []struct { - name string - wouldAutoExecute bool - includeQuestion bool - unlicensed bool - policyChecker mapPolicyChecker - wantToolStatus string - wantToolResult string - wantToolShared bool - wantFollowUp bool + name string + wouldAutoExecute bool + includeQuestion bool + unlicensed bool + policyChecker mapPolicyChecker + wantToolStatus string + wantToolResult string + wantToolUseShared bool + wantResultShared bool + wantFollowUp bool }{ { name: "interrupted all-auto batch resumes with empty accepted list", @@ -417,10 +418,11 @@ func TestHandleToolCallAutoExecutesPolicyEligiblePendingTools(t *testing.T) { policyChecker: mapPolicyChecker{ origin: {"get_issue": {policy: mcp.ToolPolicyAutoRunEverywhere, enabled: true}}, }, - wantToolStatus: conversation.StatusAutoApproved, - wantToolResult: "restored-result", - wantToolShared: true, - wantFollowUp: true, + wantToolStatus: conversation.StatusAutoApproved, + wantToolResult: "restored-result", + wantToolUseShared: true, + wantResultShared: true, + wantFollowUp: true, }, { name: "interrupted all-auto resume rejects when policy was disabled", @@ -428,10 +430,11 @@ func TestHandleToolCallAutoExecutesPolicyEligiblePendingTools(t *testing.T) { policyChecker: mapPolicyChecker{ origin: {"get_issue": {policy: mcp.ToolPolicyAutoRunEverywhere, enabled: false}}, }, - wantToolStatus: conversation.StatusRejected, - wantToolResult: "Tool call rejected by user", - wantToolShared: false, - wantFollowUp: false, // nothing executed, so nothing to follow up on + wantToolStatus: conversation.StatusRejected, + wantToolResult: "Tool call rejected by user", + wantToolUseShared: false, + wantResultShared: true, + wantFollowUp: true, }, { // Remote MCP tools are license-gated at supply time: an @@ -444,10 +447,11 @@ func TestHandleToolCallAutoExecutesPolicyEligiblePendingTools(t *testing.T) { policyChecker: mapPolicyChecker{ origin: {"get_issue": {policy: mcp.ToolPolicyAutoRunEverywhere, enabled: true}}, }, - wantToolStatus: conversation.StatusRejected, - wantToolResult: "Tool call rejected by user", - wantToolShared: false, - wantFollowUp: false, // nothing executed, so nothing to follow up on + wantToolStatus: conversation.StatusRejected, + wantToolResult: "Tool call rejected by user", + wantToolUseShared: false, + wantResultShared: true, + wantFollowUp: true, }, { name: "auto_run_everywhere policy executes on resume", @@ -456,10 +460,11 @@ func TestHandleToolCallAutoExecutesPolicyEligiblePendingTools(t *testing.T) { policyChecker: mapPolicyChecker{ origin: {"get_issue": {policy: mcp.ToolPolicyAutoRunEverywhere, enabled: true}}, }, - wantToolStatus: conversation.StatusAutoApproved, - wantToolResult: "restored-result", - wantToolShared: true, - wantFollowUp: true, + wantToolStatus: conversation.StatusAutoApproved, + wantToolResult: "restored-result", + wantToolUseShared: true, + wantResultShared: true, + wantFollowUp: true, }, { name: "policy disabled since the pause rejects instead", @@ -468,10 +473,11 @@ func TestHandleToolCallAutoExecutesPolicyEligiblePendingTools(t *testing.T) { policyChecker: mapPolicyChecker{ origin: {"get_issue": {policy: mcp.ToolPolicyAutoRunEverywhere, enabled: false}}, }, - wantToolStatus: conversation.StatusRejected, - wantToolResult: "Tool call rejected by user", - wantToolShared: false, - wantFollowUp: true, // the answered question still warrants a follow-up + wantToolStatus: conversation.StatusRejected, + wantToolResult: "Tool call rejected by user", + wantToolUseShared: false, + wantResultShared: true, + wantFollowUp: true, // the answered question still warrants a follow-up }, { name: "unmarked tool does not auto-run even if policy flipped to auto", @@ -480,10 +486,11 @@ func TestHandleToolCallAutoExecutesPolicyEligiblePendingTools(t *testing.T) { policyChecker: mapPolicyChecker{ origin: {"get_issue": {policy: mcp.ToolPolicyAutoRunEverywhere, enabled: true}}, }, - wantToolStatus: conversation.StatusRejected, - wantToolResult: "Tool call rejected by user", - wantToolShared: false, - wantFollowUp: true, + wantToolStatus: conversation.StatusRejected, + wantToolResult: "Tool call rejected by user", + wantToolUseShared: false, + wantResultShared: true, + wantFollowUp: true, }, } @@ -581,7 +588,7 @@ func TestHandleToolCallAutoExecutesPolicyEligiblePendingTools(t *testing.T) { require.NoError(t, json.Unmarshal(turns[2].Content, &updatedBlocks)) assert.Equal(t, tc.wantToolStatus, updatedBlocks[0].Status) require.NotNil(t, updatedBlocks[0].Shared) - assert.Equal(t, tc.wantToolShared, *updatedBlocks[0].Shared) + assert.Equal(t, tc.wantToolUseShared, *updatedBlocks[0].Shared) if tc.includeQuestion { assert.Equal(t, conversation.StatusSuccess, updatedBlocks[1].Status) } @@ -591,7 +598,7 @@ func TestHandleToolCallAutoExecutesPolicyEligiblePendingTools(t *testing.T) { require.Len(t, resultBlocks, len(blocks)) assert.Equal(t, tc.wantToolResult, resultBlocks[0].Content) require.NotNil(t, resultBlocks[0].Shared) - assert.Equal(t, tc.wantToolShared, *resultBlocks[0].Shared) + assert.Equal(t, tc.wantResultShared, *resultBlocks[0].Shared) assert.NotNil(t, resultBlocks[0].DecidedAt, "auto/rejected results are terminal") if tc.includeQuestion { assert.NotNil(t, resultBlocks[1].DecidedAt, "answer result is terminal") diff --git a/conversations/tool_approval.go b/conversations/tool_approval.go index 62f0e5f3a..3a35d9acf 100644 --- a/conversations/tool_approval.go +++ b/conversations/tool_approval.go @@ -277,6 +277,9 @@ func (c *Conversations) HandleToolCall(ctx context.Context, userID string, post default: rejectedToolNames = append(rejectedToolNames, block.Name) block.Status = conversation.StatusRejected + // Resume the LLM loop so the model can ask for clarification or + // take a different approach instead of silently ending. + executedAny = true toolResults = append(toolResults, toolrunner.ToolResult{ ToolCallID: block.ID, Name: block.Name, @@ -323,7 +326,10 @@ func (c *Conversations) HandleToolCall(ctx context.Context, userID string, post } // Interaction results (answered or skipped) are user-authored, so they // are terminal and shared with no separate share/keep-private step. - terminal := isDM || interactionByID[tr.ToolCallID] || autoExecutedNow[tr.ToolCallID] + // Rejected results share only the canned rejection reason; tool_use + // arguments stay unshared so they are not paraphrased into a channel reply. + rejected := toolUseStatusByID[tr.ToolCallID] == conversation.StatusRejected + terminal := isDM || interactionByID[tr.ToolCallID] || autoExecutedNow[tr.ToolCallID] || rejected rb := conversation.ContentBlock{ Type: conversation.BlockTypeToolResult, ToolUseID: tr.ToolCallID, @@ -331,7 +337,7 @@ func (c *Conversations) HandleToolCall(ctx context.Context, userID string, post Status: status, Shared: conversation.BoolPtr(terminal), } - if terminal || toolUseStatusByID[tr.ToolCallID] == conversation.StatusRejected { + if terminal { rb.DecidedAt = conversation.Int64Ptr(now) } else { needsShareDecision = true @@ -359,9 +365,9 @@ func (c *Conversations) HandleToolCall(ctx context.Context, userID string, post // In channels the follow-up is a channel-visible post that may paraphrase tool // output, so it must not stream until the requester approves sharing in - // HandleToolResult. When no share decision remains (every executed result - // was a user-interaction answer), HandleToolResult will never fire, so - // stream the follow-up now. + // HandleToolResult. When no share decision remains (every result is + // rejected, a user-interaction answer, or otherwise terminal), + // HandleToolResult will never fire, so stream the follow-up now. if !isDM && needsShareDecision { return nil } @@ -648,6 +654,9 @@ func (c *Conversations) streamToolFollowUp( if err != nil { return fmt.Errorf("failed to build completion request for tool follow-up: %w", err) } + if llm.HasRejectedToolCall(completionReq.Posts) { + completionReq.Posts = llm.EnsureToolRejectionUserMessage(completionReq.Posts) + } completionReq.Operation = llm.OperationConversationToolFollowup completionReq.OperationSubType = llm.SubTypeToolCall diff --git a/conversations/tool_approval_license_test.go b/conversations/tool_approval_license_test.go index e1e515f32..a5c1db128 100644 --- a/conversations/tool_approval_license_test.go +++ b/conversations/tool_approval_license_test.go @@ -120,13 +120,15 @@ func toolLicenseConversations(t *testing.T, convStore *loadedStateFlowStore, lic mmClient := mocks.NewMockClient(t) mmClient.On("LogDebug", mock.Anything, mock.Anything).Maybe().Return() mmClient.On("GetUser", "user-id").Return(&model.User{Id: "user-id", Username: "user"}, nil).Maybe() + mmClient.On("GetConfig").Maybe().Return(&model.Config{}) return &Conversations{ - mmClient: mmClient, - contextBuilder: toolLicenseTestBuilder(t, licensed), - bots: botsService, - licenseChecker: licenseChecker, - convService: conversation.NewService(convStore, nil, nil, nil), + mmClient: mmClient, + contextBuilder: toolLicenseTestBuilder(t, licensed), + bots: botsService, + licenseChecker: licenseChecker, + convService: conversation.NewService(convStore, nil, nil, nil), + streamingService: &loadedStateStreamingService{}, } } diff --git a/conversations/tool_rejection_followup_test.go b/conversations/tool_rejection_followup_test.go new file mode 100644 index 000000000..77ec2d087 --- /dev/null +++ b/conversations/tool_rejection_followup_test.go @@ -0,0 +1,376 @@ +// Copyright (c) 2023-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +package conversations + +import ( + "context" + "encoding/json" + "errors" + "net/http" + "strings" + "testing" + + "github.com/mattermost/mattermost-plugin-agents/v2/bots" + "github.com/mattermost/mattermost-plugin-agents/v2/conversation" + "github.com/mattermost/mattermost-plugin-agents/v2/enterprise" + "github.com/mattermost/mattermost-plugin-agents/v2/llm" + "github.com/mattermost/mattermost-plugin-agents/v2/mmapi/mocks" + "github.com/mattermost/mattermost-plugin-agents/v2/store" + "github.com/mattermost/mattermost-plugin-agents/v2/streaming" + "github.com/mattermost/mattermost/server/public/model" + "github.com/mattermost/mattermost/server/public/plugin/plugintest" + "github.com/mattermost/mattermost/server/public/pluginapi" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +const plantedRejectionArg = "SECRET-TOOL-ARG-do-not-share" + +func TestHandleToolCallRejectionFollowsUp(t *testing.T) { + dmChannel := &model.Channel{Id: "dm-channel", Type: model.ChannelTypeDirect, Name: "bot-id__user-id"} + openChannel := &model.Channel{Id: "channel-id", TeamId: "team-id", Type: model.ChannelTypeOpen} + + pendingJira := func(id string, shared bool) conversation.ContentBlock { + return conversation.ContentBlock{ + Type: conversation.BlockTypeToolUse, + ID: id, + Name: "jira__get_issue", + Input: json.RawMessage(`{"issue_key":"` + plantedRejectionArg + `"}`), + Status: conversation.StatusPending, + Shared: conversation.BoolPtr(shared), + } + } + + cases := []struct { + name string + channel *model.Channel + blocks []conversation.ContentBlock + acceptedIDs []string + failingTool bool + wantFollowUp bool + wantGuidance bool + wantRequestHas []string + wantRequestOmits []string + wantToolUseShared []bool + wantResultShared []bool + wantResultContents []string + }{ + { + name: "lone DM rejection continues with guidance", + channel: dmChannel, + blocks: []conversation.ContentBlock{pendingJira("tool-use-1", true)}, + wantFollowUp: true, + wantGuidance: true, + wantRequestHas: []string{llm.ToolRejectionUserMessage, "Tool call rejected by user"}, + wantToolUseShared: []bool{true}, + wantResultShared: []bool{true}, + wantResultContents: []string{"Tool call rejected by user"}, + }, + { + name: "lone channel rejection continues with visible reason and private args", + channel: openChannel, + blocks: []conversation.ContentBlock{pendingJira("tool-use-1", false)}, + wantFollowUp: true, + wantGuidance: true, + wantRequestHas: []string{llm.ToolRejectionUserMessage, "Tool call rejected by user"}, + wantRequestOmits: []string{plantedRejectionArg}, + wantToolUseShared: []bool{false}, + wantResultShared: []bool{true}, + wantResultContents: []string{"Tool call rejected by user"}, + }, + { + name: "mixed DM accept and reject includes rejection guidance", + channel: dmChannel, + blocks: []conversation.ContentBlock{ + pendingJira("tool-use-1", true), + { + Type: conversation.BlockTypeToolUse, + ID: "tool-use-2", + Name: "jira__transition_issue", + Input: json.RawMessage(`{"issue_key":"` + plantedRejectionArg + `"}`), + Status: conversation.StatusPending, + Shared: conversation.BoolPtr(true), + }, + }, + acceptedIDs: []string{"tool-use-1"}, + wantFollowUp: true, + wantGuidance: true, + wantRequestHas: []string{llm.ToolRejectionUserMessage, "Tool call rejected by user", "restored-result"}, + wantToolUseShared: []bool{true, true}, + wantResultShared: []bool{true, true}, + wantResultContents: []string{"restored-result", "Tool call rejected by user"}, + }, + { + name: "execution error continues without rejection guidance", + channel: dmChannel, + blocks: []conversation.ContentBlock{pendingJira("tool-use-1", true)}, + acceptedIDs: []string{"tool-use-1"}, + failingTool: true, + wantFollowUp: true, + wantGuidance: false, + wantRequestHas: []string{"jira unavailable"}, + wantRequestOmits: []string{llm.ToolRejectionUserMessage}, + wantToolUseShared: []bool{true}, + wantResultShared: []bool{true}, + wantResultContents: []string{"jira unavailable"}, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + convStore, conv := loadedStateConversationStore() + nextSeq := 1 + seedLoadToolPair(t, convStore, conv.ID, "load-1", "jira__get_issue", &nextSeq) + + content, err := json.Marshal(tc.blocks) + require.NoError(t, err) + approvalPostID := "approval-post-id" + require.NoError(t, convStore.CreateTurn(&store.Turn{ + ID: "assistant-turn", + ConversationID: conv.ID, + PostID: &approvalPostID, + Role: "assistant", + Content: content, + Sequence: nextSeq, + })) + + lm := &loadedStateLLM{} + streamingService := &loadedStateStreamingService{} + c := newRejectionFollowUpConversations(t, convStore, lm, streamingService, tc.failingTool) + + approvalPost := &model.Post{Id: approvalPostID, UserId: "bot-id"} + approvalPost.AddProp(streaming.ConversationIDProp, conv.ID) + + require.NoError(t, c.HandleToolCall(context.Background(), "user-id", approvalPost, tc.channel, tc.acceptedIDs, nil)) + streamingService.waitForStreaming() + + turns, turnsErr := convStore.GetTurnsForConversation(conv.ID) + require.NoError(t, turnsErr) + require.GreaterOrEqual(t, len(turns), 4) + + var updatedBlocks []conversation.ContentBlock + require.NoError(t, json.Unmarshal(turns[2].Content, &updatedBlocks)) + require.Len(t, updatedBlocks, len(tc.wantToolUseShared)) + for i, wantShared := range tc.wantToolUseShared { + require.NotNil(t, updatedBlocks[i].Shared) + assert.Equal(t, wantShared, *updatedBlocks[i].Shared) + } + + var resultBlocks []conversation.ContentBlock + require.NoError(t, json.Unmarshal(turns[3].Content, &resultBlocks)) + require.Len(t, resultBlocks, len(tc.wantResultContents)) + for i, wantContent := range tc.wantResultContents { + assert.Equal(t, wantContent, resultBlocks[i].Content) + require.NotNil(t, resultBlocks[i].Shared) + assert.Equal(t, tc.wantResultShared[i], *resultBlocks[i].Shared) + assert.NotNil(t, resultBlocks[i].DecidedAt) + } + + if !tc.wantFollowUp { + assert.Empty(t, lm.requests) + return + } + require.Len(t, lm.requests, 1, "expected one immediate continuation") + requestText := completionRequestText(lm.requests[0]) + if tc.wantGuidance { + assert.Equal(t, 1, countUserMessagesContaining(lm.requests[0].Posts, llm.ToolRejectionUserMessage), + "rejection guidance must appear exactly once") + } else { + assert.Zero(t, countUserMessagesContaining(lm.requests[0].Posts, llm.ToolRejectionUserMessage), + "execution errors must not receive rejection guidance") + } + for _, want := range tc.wantRequestHas { + assert.Contains(t, requestText, want) + } + for _, omit := range tc.wantRequestOmits { + assert.NotContains(t, requestText, omit) + } + }) + } +} + +func TestHandleToolCallMixedChannelRejectionGuidanceAfterShare(t *testing.T) { + convStore, conv := loadedStateConversationStore() + nextSeq := 1 + seedLoadToolPair(t, convStore, conv.ID, "load-1", "jira__get_issue", &nextSeq) + + blocks := []conversation.ContentBlock{ + { + Type: conversation.BlockTypeToolUse, + ID: "tool-use-1", + Name: "jira__get_issue", + Input: json.RawMessage(`{"issue_key":"MM-1"}`), + Status: conversation.StatusPending, + Shared: conversation.BoolPtr(false), + }, + { + Type: conversation.BlockTypeToolUse, + ID: "tool-use-2", + Name: "jira__transition_issue", + Input: json.RawMessage(`{"issue_key":"` + plantedRejectionArg + `"}`), + Status: conversation.StatusPending, + Shared: conversation.BoolPtr(false), + }, + } + content, err := json.Marshal(blocks) + require.NoError(t, err) + approvalPostID := "approval-post-id" + require.NoError(t, convStore.CreateTurn(&store.Turn{ + ID: "assistant-turn", + ConversationID: conv.ID, + PostID: &approvalPostID, + Role: "assistant", + Content: content, + Sequence: nextSeq, + })) + + lm := &loadedStateLLM{} + streamingService := &loadedStateStreamingService{} + c := newRejectionFollowUpConversations(t, convStore, lm, streamingService, false) + + approvalPost := &model.Post{Id: approvalPostID, UserId: "bot-id"} + approvalPost.AddProp(streaming.ConversationIDProp, conv.ID) + channel := &model.Channel{Id: "channel-id", TeamId: "team-id", Type: model.ChannelTypeOpen} + + require.NoError(t, c.HandleToolCall(context.Background(), "user-id", approvalPost, channel, []string{"tool-use-1"}, nil)) + assert.Empty(t, lm.requests, "mixed channel batch must wait for the share decision") + + turns, err := convStore.GetTurnsForConversation(conv.ID) + require.NoError(t, err) + var resultBlocks []conversation.ContentBlock + require.NoError(t, json.Unmarshal(turns[3].Content, &resultBlocks)) + require.Len(t, resultBlocks, 2) + assert.Nil(t, resultBlocks[0].DecidedAt) + assert.False(t, *resultBlocks[0].Shared) + assert.NotNil(t, resultBlocks[1].DecidedAt) + assert.True(t, *resultBlocks[1].Shared) + assert.Equal(t, "Tool call rejected by user", resultBlocks[1].Content) + + require.NoError(t, c.HandleToolResult(context.Background(), "user-id", approvalPost, channel, []string{"tool-use-1"})) + streamingService.waitForStreaming() + + require.Len(t, lm.requests, 1) + requestText := completionRequestText(lm.requests[0]) + assert.Equal(t, 1, countUserMessagesContaining(lm.requests[0].Posts, llm.ToolRejectionUserMessage), + "mixed rejection must receive the same guidance exactly once") + assert.Contains(t, requestText, "Tool call rejected by user") + assert.Contains(t, requestText, "restored-result") + assert.NotContains(t, requestText, plantedRejectionArg) +} + +func TestHandleToolResultRejectedOnlyDoesNotFollowUp(t *testing.T) { + convStore, conv := loadedStateConversationStore() + clickedPostID := "approval-post-id" + + assistantBlocks := []conversation.ContentBlock{{ + Type: conversation.BlockTypeToolUse, + ID: "tool-use-1", + Name: "jira__get_issue", + Input: json.RawMessage(`{"issue_key":"` + plantedRejectionArg + `"}`), + Status: conversation.StatusRejected, + Shared: conversation.BoolPtr(false), + }} + resultBlocks := []conversation.ContentBlock{{ + Type: conversation.BlockTypeToolResult, + ToolUseID: "tool-use-1", + Content: "Tool call rejected by user", + Status: conversation.StatusError, + Shared: conversation.BoolPtr(true), + }} + assistantContent, err := json.Marshal(assistantBlocks) + require.NoError(t, err) + resultContent, err := json.Marshal(resultBlocks) + require.NoError(t, err) + require.NoError(t, convStore.CreateTurn(&store.Turn{ + ID: "assistant-turn", + ConversationID: conv.ID, + PostID: &clickedPostID, + Role: "assistant", + Content: assistantContent, + Sequence: 1, + })) + require.NoError(t, convStore.CreateTurn(&store.Turn{ + ID: "result-turn", + ConversationID: conv.ID, + Role: "tool_result", + Content: resultContent, + Sequence: 2, + })) + + lm := &loadedStateLLM{} + streamingService := &loadedStateStreamingService{} + c := newRejectionFollowUpConversations(t, convStore, lm, streamingService, false) + + clickedPost := &model.Post{Id: clickedPostID, UserId: "bot-id"} + clickedPost.AddProp(streaming.ConversationIDProp, conv.ID) + channel := &model.Channel{Id: "channel-id", TeamId: "team-id", Type: model.ChannelTypeOpen} + + require.NoError(t, c.HandleToolResult(context.Background(), "user-id", clickedPost, channel, nil)) + assert.Empty(t, lm.requests, "rejected-only share-stage must not start a follow-up") +} + +func newRejectionFollowUpConversations( + t *testing.T, + convStore *loadedStateFlowStore, + lm *loadedStateLLM, + streamingService *loadedStateStreamingService, + failingTool bool, +) *Conversations { + t.Helper() + + mockAPI := &plugintest.API{} + pluginAPI := pluginapi.NewClient(mockAPI, nil) + licenseChecker := enterprise.NewLicenseChecker(pluginAPI) + botsService := bots.New(mockAPI, pluginAPI, licenseChecker, nil, nil, &http.Client{}, nil) + botsService.SetBotsForTesting([]*bots.Bot{loadedStateBot(lm)}) + + mmClient := mocks.NewMockClient(t) + mmClient.On("LogDebug", mock.Anything, mock.Anything).Maybe().Return() + mmClient.On("GetUser", "user-id").Maybe().Return(&model.User{Id: "user-id", Username: "user"}, nil) + mmClient.On("KVGet", mock.Anything, mock.Anything).Maybe().Return(nil) + mmClient.On("GetConfig").Maybe().Return(&model.Config{}) + + tools := []llm.Tool{loadedStateTool(), loadedStateTransitionTool(nil)} + if failingTool { + failing := loadedStateTool() + failing.Resolver = func(context.Context, *llm.Context, llm.ToolArgumentGetter) (string, error) { + return "", errors.New("jira unavailable") + } + tools = []llm.Tool{failing} + } + + return &Conversations{ + mmClient: mmClient, + contextBuilder: newChannelFollowUpTestBuilder(t, tools, &channelFollowUpTestConfig{}), + bots: botsService, + convService: conversation.NewService(convStore, nil, nil, nil), + streamingService: streamingService, + } +} + +func completionRequestText(req llm.CompletionRequest) string { + var b strings.Builder + for _, post := range req.Posts { + b.WriteString(post.Message) + b.WriteByte('\n') + for _, tc := range post.ToolUse { + b.Write(tc.Arguments) + b.WriteByte('\n') + b.WriteString(tc.Result) + b.WriteByte('\n') + } + } + return b.String() +} + +func countUserMessagesContaining(posts []llm.Post, msg string) int { + n := 0 + for _, post := range posts { + if post.Role == llm.PostRoleUser && strings.Contains(post.Message, msg) { + n++ + } + } + return n +} diff --git a/llm/tool_retry.go b/llm/tool_retry.go index 14629f287..eeb58ce3b 100644 --- a/llm/tool_retry.go +++ b/llm/tool_retry.go @@ -31,6 +31,11 @@ const ToolRetryLimitSystemMessage = "The last 3 tool attempts failed. Do not cal const ToolIterationLimitUserMessage = "You have used all available tool calls. Do not call any more tools. Answer the user's question using the results from your previous tool calls. If those results did not provide enough information, say so and summarize what you tried." +// ToolRejectionUserMessage is appended to a tool-follow-up request when the +// user rejected one or more pending tool calls. It must not include tool +// arguments; those stay on the (possibly redacted) tool_use blocks. +const ToolRejectionUserMessage = "The user rejected the tool call. Do not repeat the same tool call. Ask the user for clarification or choose a different approach." + // IsToolRetryExempt identifies MCP dynamic-loading meta-tools. Keep these // names in sync with mcp.SearchToolsName and mcp.LoadToolName without importing // mcp here, which would create a package cycle. @@ -66,17 +71,29 @@ func EnsureToolRetryLimitSystemMessage(posts []Post) []Post { } func EnsureToolIterationLimitUserMessage(posts []Post) []Post { - for _, post := range posts { - if post.Role == PostRoleUser && strings.Contains(post.Message, ToolIterationLimitUserMessage) { - return posts + return ensureUserMessage(posts, ToolIterationLimitUserMessage) +} + +func EnsureToolRejectionUserMessage(posts []Post) []Post { + return ensureUserMessage(posts, ToolRejectionUserMessage) +} + +// HasRejectedToolCall reports whether the most recent tool-bearing post +// includes a user-rejected tool call. Earlier rejections do not count, so a +// later successful or failed execution is not treated as a rejection follow-up. +func HasRejectedToolCall(posts []Post) bool { + for i := len(posts) - 1; i >= 0; i-- { + if len(posts[i].ToolUse) == 0 { + continue + } + for _, tc := range posts[i].ToolUse { + if tc.Status == ToolCallStatusRejected { + return true + } } + return false } - - postsCopy := append([]Post(nil), posts...) - return append(postsCopy, Post{ - Role: PostRoleUser, - Message: ToolIterationLimitUserMessage, - }) + return false } // ensureSystemMessage appends message to the first existing system post, or @@ -106,6 +123,22 @@ func ensureSystemMessage(posts []Post, message string) []Post { }}, posts...) } +// ensureUserMessage appends a user post with message when it is not already +// present on any user post. posts is returned unchanged if the message exists. +func ensureUserMessage(posts []Post, message string) []Post { + for _, post := range posts { + if post.Role == PostRoleUser && strings.Contains(post.Message, message) { + return posts + } + } + + postsCopy := append([]Post(nil), posts...) + return append(postsCopy, Post{ + Role: PostRoleUser, + Message: message, + }) +} + func trailingFailedToolCalls(toolCalls []ToolCall) (count int, allFailed bool, hasExecutedTool bool) { if len(toolCalls) == 0 { return 0, false, false diff --git a/llm/tool_retry_test.go b/llm/tool_retry_test.go index 91adaf99c..0a016f3f8 100644 --- a/llm/tool_retry_test.go +++ b/llm/tool_retry_test.go @@ -181,6 +181,54 @@ func TestEnsureToolIterationLimitUserMessage(t *testing.T) { } } +func TestEnsureToolRejectionUserMessage(t *testing.T) { + tests := []struct { + name string + posts []Post + expected []Post + }{ + { + name: "appends a user post when none exists", + posts: []Post{ + {Role: PostRoleUser, Message: "hello"}, + }, + expected: []Post{ + {Role: PostRoleUser, Message: "hello"}, + {Role: PostRoleUser, Message: ToolRejectionUserMessage}, + }, + }, + { + name: "returns posts unchanged when user message already exists", + posts: []Post{ + {Role: PostRoleUser, Message: "hello"}, + {Role: PostRoleUser, Message: ToolRejectionUserMessage}, + }, + expected: []Post{ + {Role: PostRoleUser, Message: "hello"}, + {Role: PostRoleUser, Message: ToolRejectionUserMessage}, + }, + }, + { + name: "returns posts unchanged when user message is embedded", + posts: []Post{ + {Role: PostRoleUser, Message: "hello\n\n" + ToolRejectionUserMessage}, + }, + expected: []Post{ + {Role: PostRoleUser, Message: "hello\n\n" + ToolRejectionUserMessage}, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := EnsureToolRejectionUserMessage(tt.posts) + assert.Equal(t, tt.expected, result) + assert.Equal(t, tt.expected, EnsureToolRejectionUserMessage(result), + "calling Ensure twice must not duplicate the guidance") + }) + } +} + func TestCountTrailingFailedToolCallsIgnoresFailedMetaTools(t *testing.T) { posts := []Post{{ Role: PostRoleBot, From 7b646efb5d091995477f967e4824817514acc1e9 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Wed, 26 Aug 2026 13:45:31 +0000 Subject: [PATCH 2/5] Distinguish user tool rejections Co-authored-by: Nick Misasi --- conversation/convert.go | 32 ++- conversation/convert_test.go | 59 ++++- conversations/ask_user_question_flow_test.go | 95 ++++--- conversations/tool_approval.go | 23 +- conversations/tool_approval_license_test.go | 66 +++-- conversations/tool_rejection_followup_test.go | 244 +++++++++++++++++- llm/tool_retry.go | 13 +- llm/tool_retry_test.go | 91 +++++++ 8 files changed, 527 insertions(+), 96 deletions(-) diff --git a/conversation/convert.go b/conversation/convert.go index e3c6e95a9..3c6ae9c7e 100644 --- a/conversation/convert.go +++ b/conversation/convert.go @@ -81,12 +81,14 @@ func BlocksToPost( arguments = unsharedToolUseArgumentsRedaction } toolCall := llm.ToolCall{ - ID: block.ID, - Name: block.Name, - ServerOrigin: block.ServerOrigin, - Arguments: arguments, - MCPBareName: block.MCPBareName, - Status: StatusFromString(block.Status), + ID: block.ID, + Name: block.Name, + ServerOrigin: block.ServerOrigin, + Arguments: arguments, + MCPBareName: block.MCPBareName, + Status: StatusFromString(block.Status), + UserInteraction: block.UserInteraction, + WouldAutoExecute: block.WouldAutoExecute, } if redactToolUse { toolCall.MCPBareName = "" @@ -260,14 +262,16 @@ func PostToBlocks(post llm.Post, shared bool) []ContentBlock { // 3. For each ToolUse: a tool_use block, optionally followed by a tool_result block for _, tc := range post.ToolUse { blocks = append(blocks, ContentBlock{ - Type: BlockTypeToolUse, - ID: tc.ID, - Name: tc.Name, - ServerOrigin: tc.ServerOrigin, - Input: tc.Arguments, - MCPBareName: tc.MCPBareName, - Status: StatusToString(tc.Status), - Shared: BoolPtr(shared), + Type: BlockTypeToolUse, + ID: tc.ID, + Name: tc.Name, + ServerOrigin: tc.ServerOrigin, + Input: tc.Arguments, + MCPBareName: tc.MCPBareName, + Status: StatusToString(tc.Status), + Shared: BoolPtr(shared), + UserInteraction: tc.UserInteraction, + WouldAutoExecute: tc.WouldAutoExecute, }) if tc.Result != "" { diff --git a/conversation/convert_test.go b/conversation/convert_test.go index a99560973..bd2d8fb7b 100644 --- a/conversation/convert_test.go +++ b/conversation/convert_test.go @@ -254,8 +254,10 @@ func TestPostToBlocksPreservesToolIdentityMetadata(t *testing.T) { "key": map[string]any{"type": "string"}, }, }, - MCPBareName: "get_issue", - Status: llm.ToolCallStatusPending, + MCPBareName: "get_issue", + Status: llm.ToolCallStatusPending, + UserInteraction: llm.UserInteractionSelect, + WouldAutoExecute: true, }}, } @@ -266,6 +268,8 @@ func TestPostToBlocksPreservesToolIdentityMetadata(t *testing.T) { assert.Equal(t, "jira__get_issue", blocks[0].Name) assert.Equal(t, "https://jira.example.com", blocks[0].ServerOrigin) assert.Equal(t, "get_issue", blocks[0].MCPBareName) + assert.Equal(t, llm.UserInteractionSelect, blocks[0].UserInteraction) + assert.True(t, blocks[0].WouldAutoExecute) data, err := json.Marshal(blocks[0]) require.NoError(t, err) @@ -307,6 +311,57 @@ func TestBlocksToPostRehydratesToolCatalogMetadata(t *testing.T) { assert.JSONEq(t, `{"type":"object","properties":{"key":{"type":"string"}}}`, string(toolCall.Schema.(json.RawMessage))) } +func TestBlocksToPostPreservesRejectionSignalsThroughRedaction(t *testing.T) { + blocks := []ContentBlock{ + { + Type: BlockTypeToolUse, + ID: "q-1", + Name: "AskUserQuestion", + Status: StatusRejected, + UserInteraction: llm.UserInteractionSelect, + Shared: BoolPtr(true), + }, + {Type: BlockTypeToolResult, ToolUseID: "q-1", Content: "User skipped the question", Status: StatusError, Shared: BoolPtr(true)}, + { + Type: BlockTypeToolUse, + ID: "auto-1", + Name: "jira__get_issue", + Input: json.RawMessage(`{"key":"secret"}`), + Status: StatusRejected, + WouldAutoExecute: true, + Shared: BoolPtr(false), + }, + {Type: BlockTypeToolResult, ToolUseID: "auto-1", Content: "Tool call rejected by user", Status: StatusError, Shared: BoolPtr(true)}, + { + Type: BlockTypeToolUse, + ID: "human-1", + Name: "search", + Input: json.RawMessage(`{"q":"secret"}`), + Status: StatusRejected, + Shared: BoolPtr(false), + }, + {Type: BlockTypeToolResult, ToolUseID: "human-1", Content: "Tool call rejected by user", Status: StatusError, Shared: BoolPtr(true)}, + } + + got := BlocksToPost(blocks, "assistant", PostConversionOptions{RedactUnshared: true}) + require.Len(t, got.ToolUse, 3) + + byID := map[string]llm.ToolCall{} + for _, tc := range got.ToolUse { + byID[tc.ID] = tc + } + + assert.Equal(t, llm.UserInteractionSelect, byID["q-1"].UserInteraction) + assert.False(t, byID["q-1"].WouldAutoExecute) + assert.Empty(t, byID["auto-1"].UserInteraction) + assert.True(t, byID["auto-1"].WouldAutoExecute) + assert.JSONEq(t, `{}`, string(byID["auto-1"].Arguments)) + assert.Empty(t, byID["human-1"].UserInteraction) + assert.False(t, byID["human-1"].WouldAutoExecute) + assert.JSONEq(t, `{}`, string(byID["human-1"].Arguments)) + assert.Equal(t, "Tool call rejected by user", byID["human-1"].Result) +} + func TestPostToBlocks(t *testing.T) { tests := []struct { name string diff --git a/conversations/ask_user_question_flow_test.go b/conversations/ask_user_question_flow_test.go index e1c4ba006..0bf97b5b6 100644 --- a/conversations/ask_user_question_flow_test.go +++ b/conversations/ask_user_question_flow_test.go @@ -200,6 +200,8 @@ func TestHandleToolCallAnswersUserQuestion(t *testing.T) { if tc.wantFollowUp { assert.Len(t, lm.requests, 1, "expected a follow-up LLM request") + assert.Zero(t, countUserMessagesContaining(lm.requests[0].Posts, llm.ToolRejectionUserMessage), + "answered or skipped questions must not receive tool-rejection guidance") } else { assert.Empty(t, lm.requests, "expected no follow-up LLM request") } @@ -401,16 +403,17 @@ func TestHandleToolCallAutoExecutesPolicyEligiblePendingTools(t *testing.T) { const origin = "https://jira.example.com" cases := []struct { - name string - wouldAutoExecute bool - includeQuestion bool - unlicensed bool - policyChecker mapPolicyChecker - wantToolStatus string - wantToolResult string - wantToolUseShared bool - wantResultShared bool - wantFollowUp bool + name string + wouldAutoExecute bool + includeQuestion bool + unlicensed bool + policyChecker mapPolicyChecker + wantToolStatus string + wantToolResult string + wantToolUseShared bool + wantResultShared bool + wantFollowUp bool + wantRejectionGuidance bool }{ { name: "interrupted all-auto batch resumes with empty accepted list", @@ -418,11 +421,12 @@ func TestHandleToolCallAutoExecutesPolicyEligiblePendingTools(t *testing.T) { policyChecker: mapPolicyChecker{ origin: {"get_issue": {policy: mcp.ToolPolicyAutoRunEverywhere, enabled: true}}, }, - wantToolStatus: conversation.StatusAutoApproved, - wantToolResult: "restored-result", - wantToolUseShared: true, - wantResultShared: true, - wantFollowUp: true, + wantToolStatus: conversation.StatusAutoApproved, + wantToolResult: "restored-result", + wantToolUseShared: true, + wantResultShared: true, + wantFollowUp: true, + wantRejectionGuidance: false, }, { name: "interrupted all-auto resume rejects when policy was disabled", @@ -430,11 +434,12 @@ func TestHandleToolCallAutoExecutesPolicyEligiblePendingTools(t *testing.T) { policyChecker: mapPolicyChecker{ origin: {"get_issue": {policy: mcp.ToolPolicyAutoRunEverywhere, enabled: false}}, }, - wantToolStatus: conversation.StatusRejected, - wantToolResult: "Tool call rejected by user", - wantToolUseShared: false, - wantResultShared: true, - wantFollowUp: true, + wantToolStatus: conversation.StatusRejected, + wantToolResult: "Tool call rejected by user", + wantToolUseShared: false, + wantResultShared: true, + wantFollowUp: true, + wantRejectionGuidance: false, }, { // Remote MCP tools are license-gated at supply time: an @@ -447,11 +452,12 @@ func TestHandleToolCallAutoExecutesPolicyEligiblePendingTools(t *testing.T) { policyChecker: mapPolicyChecker{ origin: {"get_issue": {policy: mcp.ToolPolicyAutoRunEverywhere, enabled: true}}, }, - wantToolStatus: conversation.StatusRejected, - wantToolResult: "Tool call rejected by user", - wantToolUseShared: false, - wantResultShared: true, - wantFollowUp: true, + wantToolStatus: conversation.StatusRejected, + wantToolResult: "Tool call rejected by user", + wantToolUseShared: false, + wantResultShared: true, + wantFollowUp: true, + wantRejectionGuidance: false, }, { name: "auto_run_everywhere policy executes on resume", @@ -460,11 +466,12 @@ func TestHandleToolCallAutoExecutesPolicyEligiblePendingTools(t *testing.T) { policyChecker: mapPolicyChecker{ origin: {"get_issue": {policy: mcp.ToolPolicyAutoRunEverywhere, enabled: true}}, }, - wantToolStatus: conversation.StatusAutoApproved, - wantToolResult: "restored-result", - wantToolUseShared: true, - wantResultShared: true, - wantFollowUp: true, + wantToolStatus: conversation.StatusAutoApproved, + wantToolResult: "restored-result", + wantToolUseShared: true, + wantResultShared: true, + wantFollowUp: true, + wantRejectionGuidance: false, }, { name: "policy disabled since the pause rejects instead", @@ -473,11 +480,12 @@ func TestHandleToolCallAutoExecutesPolicyEligiblePendingTools(t *testing.T) { policyChecker: mapPolicyChecker{ origin: {"get_issue": {policy: mcp.ToolPolicyAutoRunEverywhere, enabled: false}}, }, - wantToolStatus: conversation.StatusRejected, - wantToolResult: "Tool call rejected by user", - wantToolUseShared: false, - wantResultShared: true, - wantFollowUp: true, // the answered question still warrants a follow-up + wantToolStatus: conversation.StatusRejected, + wantToolResult: "Tool call rejected by user", + wantToolUseShared: false, + wantResultShared: true, + wantFollowUp: true, + wantRejectionGuidance: false, }, { name: "unmarked tool does not auto-run even if policy flipped to auto", @@ -486,11 +494,12 @@ func TestHandleToolCallAutoExecutesPolicyEligiblePendingTools(t *testing.T) { policyChecker: mapPolicyChecker{ origin: {"get_issue": {policy: mcp.ToolPolicyAutoRunEverywhere, enabled: true}}, }, - wantToolStatus: conversation.StatusRejected, - wantToolResult: "Tool call rejected by user", - wantToolUseShared: false, - wantResultShared: true, - wantFollowUp: true, + wantToolStatus: conversation.StatusRejected, + wantToolResult: "Tool call rejected by user", + wantToolUseShared: false, + wantResultShared: true, + wantFollowUp: true, + wantRejectionGuidance: true, }, } @@ -606,6 +615,12 @@ func TestHandleToolCallAutoExecutesPolicyEligiblePendingTools(t *testing.T) { if tc.wantFollowUp { assert.Len(t, lm.requests, 1, "expected a follow-up LLM request") + if tc.wantRejectionGuidance { + requireRejectionGuidanceIsFinalUserPost(t, lm.requests[0].Posts) + } else { + assert.Zero(t, countUserMessagesContaining(lm.requests[0].Posts, llm.ToolRejectionUserMessage), + "policy denial and auto-exec must not receive user-rejection guidance") + } } else { assert.Empty(t, lm.requests) } diff --git a/conversations/tool_approval.go b/conversations/tool_approval.go index 3a35d9acf..cad639063 100644 --- a/conversations/tool_approval.go +++ b/conversations/tool_approval.go @@ -183,7 +183,10 @@ func (c *Conversations) HandleToolCall(ctx context.Context, userID string, post // neither audit list; anything auto-executed this call counts as accepted. autoExec := c.shouldAutoExecuteTool(llmContext, isDM) autoExecutedNow := make(map[string]bool) - executedAny := false + // resolvedAny is true when this click resolved at least one pending + // tool_use (executed, skipped, auto-run, or rejected). It is the + // follow-up gate, not a record of side-effecting execution. + resolvedAny := false acceptedToolNames := []string{} rejectedToolNames := []string{} var toolResults []toolrunner.ToolResult @@ -203,7 +206,7 @@ func (c *Conversations) HandleToolCall(ctx context.Context, userID string, post // Shared so the channel-visible follow-up may reference the answer. block.Status = conversation.StatusSuccess block.Shared = conversation.BoolPtr(true) - executedAny = true + resolvedAny = true toolResults = append(toolResults, toolrunner.ToolResult{ ToolCallID: block.ID, Name: block.Name, @@ -213,7 +216,7 @@ func (c *Conversations) HandleToolCall(ctx context.Context, userID string, post case slices.Contains(acceptedToolIDs, block.ID): acceptedToolNames = append(acceptedToolNames, block.Name) result, resolveErr := resolveApprovedToolUseBlock(ctx, llmContext, *block) - executedAny = true + resolvedAny = true if resolveErr != nil { block.Status = conversation.StatusError toolResults = append(toolResults, toolrunner.ToolResult{ @@ -236,10 +239,11 @@ func (c *Conversations) HandleToolCall(ctx context.Context, userID string, post // Skipped question: record the decline as the result and stream a // follow-up so the model can proceed without the answer, per the // tool contract. Shared because the decline is user-authored, not - // private tool output. + // private tool output. UserInteraction stays on the block so the + // follow-up can distinguish this from a regular tool rejection. block.Status = conversation.StatusRejected block.Shared = conversation.BoolPtr(true) - executedAny = true + resolvedAny = true toolResults = append(toolResults, toolrunner.ToolResult{ ToolCallID: block.ID, Name: block.Name, @@ -255,7 +259,7 @@ func (c *Conversations) HandleToolCall(ctx context.Context, userID string, post // any auto-run round. result, resolveErr := resolveApprovedToolUseBlock(ctx, llmContext, *block) autoExecutedNow[block.ID] = true - executedAny = true + resolvedAny = true block.Shared = conversation.BoolPtr(true) if resolveErr != nil { block.Status = conversation.StatusError @@ -279,7 +283,7 @@ func (c *Conversations) HandleToolCall(ctx context.Context, userID string, post block.Status = conversation.StatusRejected // Resume the LLM loop so the model can ask for clarification or // take a different approach instead of silently ending. - executedAny = true + resolvedAny = true toolResults = append(toolResults, toolrunner.ToolResult{ ToolCallID: block.ID, Name: block.Name, @@ -359,7 +363,7 @@ func (c *Conversations) HandleToolCall(ctx context.Context, userID string, post return fmt.Errorf("failed to create tool result turn: %w", err) } - if !executedAny { + if !resolvedAny { return nil } @@ -585,7 +589,8 @@ func (c *Conversations) HandleToolResult(ctx context.Context, userID string, pos // Only stream a follow-up when there is something to follow up on: // at least one executed tool_result exists on this post. Rejected-only - // posts produce no output worth streaming. + // posts already streamed their continuation from HandleToolCall (no + // share decision remains), so a second-stage click must not stream again. if !clickedPostHasExecutedTool { return nil } diff --git a/conversations/tool_approval_license_test.go b/conversations/tool_approval_license_test.go index a5c1db128..cfebcb646 100644 --- a/conversations/tool_approval_license_test.go +++ b/conversations/tool_approval_license_test.go @@ -58,7 +58,10 @@ func (p *toolLicenseBuiltinProvider) GetTools(*bots.Bot, *llm.Context) []llm.Too // toolLicenseTestBot returns a bot without dynamic MCP tool loading so every // provided tool is immediately resolvable in the visible tool store. -func toolLicenseTestBot() *bots.Bot { +func toolLicenseTestBot(lm llm.LanguageModel) *bots.Bot { + if lm == nil { + lm = &loadedStateLLM{} + } return bots.NewBot( llm.BotConfig{ ID: "bot-id", @@ -70,7 +73,7 @@ func toolLicenseTestBot() *bots.Bot { }, llm.ServiceConfig{DefaultModel: "test-model", Type: llm.ServiceTypeOpenAI}, &model.Bot{UserId: "bot-id", Username: "matty", DisplayName: "Matty"}, - &loadedStateLLM{}, + lm, ) } @@ -108,14 +111,16 @@ func toolLicenseTestBuilder(t *testing.T, licensed bool) *llmcontext.Builder { ) } -func toolLicenseConversations(t *testing.T, convStore *loadedStateFlowStore, licensed bool) *Conversations { +func toolLicenseConversations(t *testing.T, convStore *loadedStateFlowStore, licensed bool) (*Conversations, *loadedStateLLM, *loadedStateStreamingService) { t.Helper() mockAPI := &plugintest.API{} pluginAPI := pluginapi.NewClient(mockAPI, nil) licenseChecker := toolLicenseChecker(t, licensed) botsService := bots.New(mockAPI, pluginAPI, licenseChecker, nil, nil, &http.Client{}, nil) - botsService.SetBotsForTesting([]*bots.Bot{toolLicenseTestBot()}) + lm := &loadedStateLLM{} + streamingService := &loadedStateStreamingService{} + botsService.SetBotsForTesting([]*bots.Bot{toolLicenseTestBot(lm)}) mmClient := mocks.NewMockClient(t) mmClient.On("LogDebug", mock.Anything, mock.Anything).Maybe().Return() @@ -128,8 +133,8 @@ func toolLicenseConversations(t *testing.T, convStore *loadedStateFlowStore, lic bots: botsService, licenseChecker: licenseChecker, convService: conversation.NewService(convStore, nil, nil, nil), - streamingService: &loadedStateStreamingService{}, - } + streamingService: streamingService, + }, lm, streamingService } // TestHandleToolCallLicenseGate pins the license split for tool approvals: @@ -138,14 +143,16 @@ func toolLicenseConversations(t *testing.T, convStore *loadedStateFlowStore, lic // MCP servers require one to execute. Rejections never require a license. func TestHandleToolCallLicenseGate(t *testing.T) { tests := []struct { - name string - toolName string - origin string - licensed bool - accept bool - wantErr error - wantStatus string - wantResult string + name string + toolName string + origin string + licensed bool + accept bool + wantErr error + wantStatus string + wantResult string + wantFollowUp bool + wantRejectionGuidance bool }{ { name: "embedded MCP tool executes without license", @@ -183,13 +190,15 @@ func TestHandleToolCallLicenseGate(t *testing.T) { wantResult: "mcp:jira__get_issue", }, { - name: "remote MCP tool rejection is allowed without license", - toolName: "jira__get_issue", - origin: toolLicenseRemoteOrigin, - licensed: false, - accept: false, - wantStatus: conversation.StatusRejected, - wantResult: "Tool call rejected by user", + name: "remote MCP tool rejection is allowed without license", + toolName: "jira__get_issue", + origin: toolLicenseRemoteOrigin, + licensed: false, + accept: false, + wantStatus: conversation.StatusRejected, + wantResult: "Tool call rejected by user", + wantFollowUp: true, + wantRejectionGuidance: true, }, } @@ -217,7 +226,7 @@ func TestHandleToolCallLicenseGate(t *testing.T) { Sequence: 1, })) - c := toolLicenseConversations(t, convStore, tc.licensed) + c, lm, streamingService := toolLicenseConversations(t, convStore, tc.licensed) approvalPost := &model.Post{Id: approvalPostID, UserId: "bot-id"} approvalPost.AddProp(streaming.ConversationIDProp, conv.ID) @@ -229,6 +238,7 @@ func TestHandleToolCallLicenseGate(t *testing.T) { } err = c.HandleToolCall(context.Background(), "user-id", approvalPost, channel, acceptedIDs, nil) + streamingService.waitForStreaming() turns, turnsErr := convStore.GetTurnsForConversation(conv.ID) require.NoError(t, turnsErr) @@ -242,6 +252,7 @@ func TestHandleToolCallLicenseGate(t *testing.T) { var untouched []conversation.ContentBlock require.NoError(t, json.Unmarshal(turns[0].Content, &untouched)) require.Equal(t, conversation.StatusPending, untouched[0].Status) + require.Empty(t, lm.requests) return } @@ -256,6 +267,15 @@ func TestHandleToolCallLicenseGate(t *testing.T) { require.NoError(t, json.Unmarshal(turns[1].Content, &resultBlocks)) require.Equal(t, conversation.BlockTypeToolResult, resultBlocks[0].Type) require.Equal(t, tc.wantResult, resultBlocks[0].Content) + + if tc.wantFollowUp { + require.Len(t, lm.requests, 1, "rejection continuation must start one follow-up") + if tc.wantRejectionGuidance { + requireRejectionGuidanceIsFinalUserPost(t, lm.requests[0].Posts) + } + } else { + require.Empty(t, lm.requests) + } }) } } @@ -348,7 +368,7 @@ func TestHandleToolResultLicenseGate(t *testing.T) { Sequence: 2, })) - c := toolLicenseConversations(t, convStore, tc.licensed) + c, _, _ := toolLicenseConversations(t, convStore, tc.licensed) resultPost := &model.Post{Id: resultPostID, UserId: "bot-id"} resultPost.AddProp(streaming.ConversationIDProp, conv.ID) diff --git a/conversations/tool_rejection_followup_test.go b/conversations/tool_rejection_followup_test.go index 77ec2d087..7bb4334e8 100644 --- a/conversations/tool_rejection_followup_test.go +++ b/conversations/tool_rejection_followup_test.go @@ -7,6 +7,7 @@ import ( "context" "encoding/json" "errors" + "fmt" "net/http" "strings" "testing" @@ -116,6 +117,29 @@ func TestHandleToolCallRejectionFollowsUp(t *testing.T) { wantResultShared: []bool{true}, wantResultContents: []string{"jira unavailable"}, }, + { + name: "skipped AskUserQuestion continues without rejection guidance", + channel: dmChannel, + blocks: []conversation.ContentBlock{{ + Type: conversation.BlockTypeToolUse, + ID: "q-1", + Name: "AskUserQuestion", + Input: json.RawMessage(`{ + "question": "Which channel should I post in?", + "options": [{"label": "UX Design"}, {"label": "Design team"}] + }`), + Status: conversation.StatusPending, + UserInteraction: llm.UserInteractionSelect, + Shared: conversation.BoolPtr(true), + }}, + wantFollowUp: true, + wantGuidance: false, + wantRequestHas: []string{"User skipped the question"}, + wantRequestOmits: []string{llm.ToolRejectionUserMessage}, + wantToolUseShared: []bool{true}, + wantResultShared: []bool{true}, + wantResultContents: []string{"User skipped the question"}, + }, } for _, tc := range cases { @@ -175,11 +199,10 @@ func TestHandleToolCallRejectionFollowsUp(t *testing.T) { require.Len(t, lm.requests, 1, "expected one immediate continuation") requestText := completionRequestText(lm.requests[0]) if tc.wantGuidance { - assert.Equal(t, 1, countUserMessagesContaining(lm.requests[0].Posts, llm.ToolRejectionUserMessage), - "rejection guidance must appear exactly once") + requireRejectionGuidanceIsFinalUserPost(t, lm.requests[0].Posts) } else { assert.Zero(t, countUserMessagesContaining(lm.requests[0].Posts, llm.ToolRejectionUserMessage), - "execution errors must not receive rejection guidance") + "non-human-rejection continuations must not receive rejection guidance") } for _, want := range tc.wantRequestHas { assert.Contains(t, requestText, want) @@ -253,8 +276,7 @@ func TestHandleToolCallMixedChannelRejectionGuidanceAfterShare(t *testing.T) { require.Len(t, lm.requests, 1) requestText := completionRequestText(lm.requests[0]) - assert.Equal(t, 1, countUserMessagesContaining(lm.requests[0].Posts, llm.ToolRejectionUserMessage), - "mixed rejection must receive the same guidance exactly once") + requireRejectionGuidanceIsFinalUserPost(t, lm.requests[0].Posts) assert.Contains(t, requestText, "Tool call rejected by user") assert.Contains(t, requestText, "restored-result") assert.NotContains(t, requestText, plantedRejectionArg) @@ -311,6 +333,218 @@ func TestHandleToolResultRejectedOnlyDoesNotFollowUp(t *testing.T) { assert.Empty(t, lm.requests, "rejected-only share-stage must not start a follow-up") } +func TestStreamToolFollowUpLatestToolBatchGuidance(t *testing.T) { + humanReject := conversation.ContentBlock{ + Type: conversation.BlockTypeToolUse, + ID: "human-1", + Name: "jira__get_issue", + Input: json.RawMessage(`{}`), + Status: conversation.StatusRejected, + Shared: conversation.BoolPtr(true), + } + humanRejectResult := conversation.ContentBlock{ + Type: conversation.BlockTypeToolResult, + ToolUseID: "human-1", + Content: "Tool call rejected by user", + Status: conversation.StatusError, + Shared: conversation.BoolPtr(true), + } + success := conversation.ContentBlock{ + Type: conversation.BlockTypeToolUse, + ID: "ok-1", + Name: "jira__get_issue", + Input: json.RawMessage(`{}`), + Status: conversation.StatusSuccess, + Shared: conversation.BoolPtr(true), + } + successResult := conversation.ContentBlock{ + Type: conversation.BlockTypeToolResult, + ToolUseID: "ok-1", + Content: "restored-result", + Status: conversation.StatusSuccess, + Shared: conversation.BoolPtr(true), + } + execError := conversation.ContentBlock{ + Type: conversation.BlockTypeToolUse, + ID: "err-1", + Name: "jira__get_issue", + Input: json.RawMessage(`{}`), + Status: conversation.StatusError, + Shared: conversation.BoolPtr(true), + } + execErrorResult := conversation.ContentBlock{ + Type: conversation.BlockTypeToolResult, + ToolUseID: "err-1", + Content: "jira unavailable", + Status: conversation.StatusError, + Shared: conversation.BoolPtr(true), + } + skip := conversation.ContentBlock{ + Type: conversation.BlockTypeToolUse, + ID: "q-1", + Name: "AskUserQuestion", + Status: conversation.StatusRejected, + UserInteraction: llm.UserInteractionSelect, + Shared: conversation.BoolPtr(true), + } + skipResult := conversation.ContentBlock{ + Type: conversation.BlockTypeToolResult, + ToolUseID: "q-1", + Content: "User skipped the question", + Status: conversation.StatusError, + Shared: conversation.BoolPtr(true), + } + policyDenied := conversation.ContentBlock{ + Type: conversation.BlockTypeToolUse, + ID: "auto-1", + Name: "jira__get_issue", + Input: json.RawMessage(`{}`), + Status: conversation.StatusRejected, + WouldAutoExecute: true, + Shared: conversation.BoolPtr(true), + } + policyDeniedResult := conversation.ContentBlock{ + Type: conversation.BlockTypeToolResult, + ToolUseID: "auto-1", + Content: "Tool call rejected by user", + Status: conversation.StatusError, + Shared: conversation.BoolPtr(true), + } + + cases := []struct { + name string + rounds [][]conversation.ContentBlock + wantGuidance bool + }{ + { + name: "empty conversation", + wantGuidance: false, + }, + { + name: "success only", + rounds: [][]conversation.ContentBlock{{success, successResult}}, + wantGuidance: false, + }, + { + name: "execution error only", + rounds: [][]conversation.ContentBlock{{execError, execErrorResult}}, + wantGuidance: false, + }, + { + name: "interaction skip only", + rounds: [][]conversation.ContentBlock{{skip, skipResult}}, + wantGuidance: false, + }, + { + name: "policy-denied auto-exec only", + rounds: [][]conversation.ContentBlock{{policyDenied, policyDeniedResult}}, + wantGuidance: false, + }, + { + name: "latest mixed success and human rejection", + rounds: [][]conversation.ContentBlock{{success, humanReject, successResult, humanRejectResult}}, + wantGuidance: true, + }, + { + name: "older human rejection then later success", + rounds: [][]conversation.ContentBlock{ + {humanReject, humanRejectResult}, + {success, successResult}, + }, + wantGuidance: false, + }, + { + name: "older human rejection then later execution error", + rounds: [][]conversation.ContentBlock{ + {humanReject, humanRejectResult}, + {execError, execErrorResult}, + }, + wantGuidance: false, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + convStore, conv := loadedStateConversationStore() + seq := 1 + for i, blocks := range tc.rounds { + appendToolRoundTurns(t, convStore, conv.ID, fmt.Sprintf("post-%d", i+1), &seq, blocks) + } + + lm := &loadedStateLLM{} + streamingService := &loadedStateStreamingService{} + c := newRejectionFollowUpConversations(t, convStore, lm, streamingService, false) + + err := c.streamToolFollowUp( + context.Background(), + loadedStateBot(lm), + &model.User{Id: "user-id", Username: "user"}, + &model.Channel{Id: "dm-channel", Type: model.ChannelTypeDirect, Name: "bot-id__user-id"}, + &model.Post{Id: "post-1"}, + conv, + true, + nil, + ) + require.NoError(t, err) + streamingService.waitForStreaming() + require.Len(t, lm.requests, 1) + if tc.wantGuidance { + requireRejectionGuidanceIsFinalUserPost(t, lm.requests[0].Posts) + } else { + assert.Zero(t, countUserMessagesContaining(lm.requests[0].Posts, llm.ToolRejectionUserMessage)) + } + }) + } +} + +func appendToolRoundTurns(t *testing.T, convStore *loadedStateFlowStore, convID, postID string, seq *int, blocks []conversation.ContentBlock) { + t.Helper() + + var toolUses, results []conversation.ContentBlock + for _, block := range blocks { + switch block.Type { + case conversation.BlockTypeToolUse: + toolUses = append(toolUses, block) + case conversation.BlockTypeToolResult: + results = append(results, block) + } + } + + useContent, err := json.Marshal(toolUses) + require.NoError(t, err) + postIDCopy := postID + require.NoError(t, convStore.CreateTurn(&store.Turn{ + ID: "assistant-" + postID, + ConversationID: convID, + PostID: &postIDCopy, + Role: "assistant", + Content: useContent, + Sequence: *seq, + })) + *seq++ + + resultContent, err := json.Marshal(results) + require.NoError(t, err) + require.NoError(t, convStore.CreateTurn(&store.Turn{ + ID: "result-" + postID, + ConversationID: convID, + Role: "tool_result", + Content: resultContent, + Sequence: *seq, + })) + *seq++ +} + +func requireRejectionGuidanceIsFinalUserPost(t *testing.T, posts []llm.Post) { + t.Helper() + require.NotEmpty(t, posts) + assert.Equal(t, 1, countUserMessagesContaining(posts, llm.ToolRejectionUserMessage), + "rejection guidance must appear exactly once") + last := posts[len(posts)-1] + require.Equal(t, llm.PostRoleUser, last.Role, "rejection guidance must be the final user post") + require.Equal(t, llm.ToolRejectionUserMessage, last.Message) +} + func newRejectionFollowUpConversations( t *testing.T, convStore *loadedStateFlowStore, diff --git a/llm/tool_retry.go b/llm/tool_retry.go index eeb58ce3b..93b78af88 100644 --- a/llm/tool_retry.go +++ b/llm/tool_retry.go @@ -79,15 +79,18 @@ func EnsureToolRejectionUserMessage(posts []Post) []Post { } // HasRejectedToolCall reports whether the most recent tool-bearing post -// includes a user-rejected tool call. Earlier rejections do not count, so a -// later successful or failed execution is not treated as a rejection follow-up. +// includes a human rejection of a regular tool. Earlier rejections do not +// count, so a later success or execution error is not treated as a rejection +// follow-up. Skipped user-interaction tools (UserInteraction set) and +// policy-denied auto-exec tools (WouldAutoExecute still set) are not human +// rejections. func HasRejectedToolCall(posts []Post) bool { for i := len(posts) - 1; i >= 0; i-- { if len(posts[i].ToolUse) == 0 { continue } for _, tc := range posts[i].ToolUse { - if tc.Status == ToolCallStatusRejected { + if isHumanRejectedToolCall(tc) { return true } } @@ -96,6 +99,10 @@ func HasRejectedToolCall(posts []Post) bool { return false } +func isHumanRejectedToolCall(tc ToolCall) bool { + return tc.Status == ToolCallStatusRejected && tc.UserInteraction == "" && !tc.WouldAutoExecute +} + // ensureSystemMessage appends message to the first existing system post, or // prepends a new system post if none exists. If the message is already present // on a system post, posts is returned unchanged. diff --git a/llm/tool_retry_test.go b/llm/tool_retry_test.go index 0a016f3f8..a7a675128 100644 --- a/llm/tool_retry_test.go +++ b/llm/tool_retry_test.go @@ -229,6 +229,97 @@ func TestEnsureToolRejectionUserMessage(t *testing.T) { } } +func TestHasRejectedToolCall(t *testing.T) { + humanReject := ToolCall{ID: "r1", Name: "search", Status: ToolCallStatusRejected} + skip := ToolCall{ID: "q1", Name: "AskUserQuestion", Status: ToolCallStatusRejected, UserInteraction: UserInteractionSelect} + policyDenied := ToolCall{ID: "a1", Name: "jira__get_issue", Status: ToolCallStatusRejected, WouldAutoExecute: true} + success := ToolCall{ID: "s1", Name: "search", Status: ToolCallStatusSuccess} + execError := ToolCall{ID: "e1", Name: "search", Status: ToolCallStatusError} + + tests := []struct { + name string + posts []Post + expected bool + }{ + { + name: "empty posts", + posts: nil, + expected: false, + }, + { + name: "success only", + posts: []Post{{Role: PostRoleBot, ToolUse: []ToolCall{success}}}, + expected: false, + }, + { + name: "execution error only", + posts: []Post{{Role: PostRoleBot, ToolUse: []ToolCall{execError}}}, + expected: false, + }, + { + name: "interaction skip only", + posts: []Post{{Role: PostRoleBot, ToolUse: []ToolCall{skip}}}, + expected: false, + }, + { + name: "policy-denied auto-exec is not a human rejection", + posts: []Post{{Role: PostRoleBot, ToolUse: []ToolCall{policyDenied}}}, + expected: false, + }, + { + name: "human rejection", + posts: []Post{{Role: PostRoleBot, ToolUse: []ToolCall{humanReject}}}, + expected: true, + }, + { + name: "latest mixed success and human rejection", + posts: []Post{{ + Role: PostRoleBot, + ToolUse: []ToolCall{success, humanReject}, + }}, + expected: true, + }, + { + name: "latest mixed success and interaction skip", + posts: []Post{{ + Role: PostRoleBot, + ToolUse: []ToolCall{success, skip}, + }}, + expected: false, + }, + { + name: "older human rejection then later success", + posts: []Post{ + {Role: PostRoleBot, ToolUse: []ToolCall{humanReject}}, + {Role: PostRoleBot, ToolUse: []ToolCall{success}}, + }, + expected: false, + }, + { + name: "older human rejection then later execution error", + posts: []Post{ + {Role: PostRoleBot, ToolUse: []ToolCall{humanReject}}, + {Role: PostRoleBot, ToolUse: []ToolCall{execError}}, + }, + expected: false, + }, + { + name: "trailing user post does not hide the latest tool-bearing rejection", + posts: []Post{ + {Role: PostRoleBot, ToolUse: []ToolCall{humanReject}}, + {Role: PostRoleUser, Message: "thanks"}, + }, + expected: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.expected, HasRejectedToolCall(tt.posts)) + }) + } +} + func TestCountTrailingFailedToolCallsIgnoresFailedMetaTools(t *testing.T) { posts := []Post{{ Role: PostRoleBot, From 4b6dcc1b3b9065fc1e505859a323d6ebacf45a7a Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Wed, 26 Aug 2026 18:56:54 +0000 Subject: [PATCH 3/5] Distinguish policy denial from user tool rejection Co-authored-by: mattermost-code --- conversations/ask_user_question_flow_test.go | 21 ++++++---- conversations/tool_approval.go | 29 +++++++++++-- conversations/tool_approval_license_test.go | 2 +- conversations/tool_rejection_followup_test.go | 42 ++++++++++++++----- 4 files changed, 72 insertions(+), 22 deletions(-) diff --git a/conversations/ask_user_question_flow_test.go b/conversations/ask_user_question_flow_test.go index 0bf97b5b6..8966b23a0 100644 --- a/conversations/ask_user_question_flow_test.go +++ b/conversations/ask_user_question_flow_test.go @@ -397,8 +397,8 @@ func TestStreamToolFollowUpInteractiveFlag(t *testing.T) { // TestHandleToolCallAutoExecutesPolicyEligiblePendingTools pins the deferred // auto-execution contract: marked tools run server-side without appearing in // accepted_tool_ids, including when an interrupted all-auto batch is resumed -// with an empty list. A policy disabled since the pause must fall back to -// rejection. +// with an empty list. A policy or license change since the pause must fall +// back to a non-user-rejection result so the follow-up cannot blame the user. func TestHandleToolCallAutoExecutesPolicyEligiblePendingTools(t *testing.T) { const origin = "https://jira.example.com" @@ -435,7 +435,7 @@ func TestHandleToolCallAutoExecutesPolicyEligiblePendingTools(t *testing.T) { origin: {"get_issue": {policy: mcp.ToolPolicyAutoRunEverywhere, enabled: false}}, }, wantToolStatus: conversation.StatusRejected, - wantToolResult: "Tool call rejected by user", + wantToolResult: toolCallPolicyDeniedResult, wantToolUseShared: false, wantResultShared: true, wantFollowUp: true, @@ -453,7 +453,7 @@ func TestHandleToolCallAutoExecutesPolicyEligiblePendingTools(t *testing.T) { origin: {"get_issue": {policy: mcp.ToolPolicyAutoRunEverywhere, enabled: true}}, }, wantToolStatus: conversation.StatusRejected, - wantToolResult: "Tool call rejected by user", + wantToolResult: toolCallPolicyDeniedResult, wantToolUseShared: false, wantResultShared: true, wantFollowUp: true, @@ -481,7 +481,7 @@ func TestHandleToolCallAutoExecutesPolicyEligiblePendingTools(t *testing.T) { origin: {"get_issue": {policy: mcp.ToolPolicyAutoRunEverywhere, enabled: false}}, }, wantToolStatus: conversation.StatusRejected, - wantToolResult: "Tool call rejected by user", + wantToolResult: toolCallPolicyDeniedResult, wantToolUseShared: false, wantResultShared: true, wantFollowUp: true, @@ -495,7 +495,7 @@ func TestHandleToolCallAutoExecutesPolicyEligiblePendingTools(t *testing.T) { origin: {"get_issue": {policy: mcp.ToolPolicyAutoRunEverywhere, enabled: true}}, }, wantToolStatus: conversation.StatusRejected, - wantToolResult: "Tool call rejected by user", + wantToolResult: toolCallRejectedByUserResult, wantToolUseShared: false, wantResultShared: true, wantFollowUp: true, @@ -596,6 +596,8 @@ func TestHandleToolCallAutoExecutesPolicyEligiblePendingTools(t *testing.T) { var updatedBlocks []conversation.ContentBlock require.NoError(t, json.Unmarshal(turns[2].Content, &updatedBlocks)) assert.Equal(t, tc.wantToolStatus, updatedBlocks[0].Status) + assert.Equal(t, tc.wouldAutoExecute, updatedBlocks[0].WouldAutoExecute, + "WouldAutoExecute must stay on the block so follow-up guidance can distinguish policy denial from a user rejection") require.NotNil(t, updatedBlocks[0].Shared) assert.Equal(t, tc.wantToolUseShared, *updatedBlocks[0].Shared) if tc.includeQuestion { @@ -614,12 +616,17 @@ func TestHandleToolCallAutoExecutesPolicyEligiblePendingTools(t *testing.T) { } if tc.wantFollowUp { - assert.Len(t, lm.requests, 1, "expected a follow-up LLM request") + require.Len(t, lm.requests, 1, "expected a follow-up LLM request") + requestText := completionRequestText(lm.requests[0]) + assert.Contains(t, requestText, tc.wantToolResult) if tc.wantRejectionGuidance { requireRejectionGuidanceIsFinalUserPost(t, lm.requests[0].Posts) } else { assert.Zero(t, countUserMessagesContaining(lm.requests[0].Posts, llm.ToolRejectionUserMessage), "policy denial and auto-exec must not receive user-rejection guidance") + if tc.wantToolResult != toolCallRejectedByUserResult { + assert.NotContains(t, requestText, toolCallRejectedByUserResult) + } } } else { assert.Empty(t, lm.requests) diff --git a/conversations/tool_approval.go b/conversations/tool_approval.go index cad639063..6f42acdf2 100644 --- a/conversations/tool_approval.go +++ b/conversations/tool_approval.go @@ -61,6 +61,14 @@ var ErrInvalidToolAnswer = errors.New("invalid answer for user interaction tool // entirely on unlicensed servers. var ErrRemoteMCPNotLicensed = errors.New("tools from remote MCP servers require a license with MCP support") +// Canned tool_result content for non-executing resolutions. These are shown +// to the LLM (and, when terminal, in channels); they must not include tool +// arguments or blame the user for an administrative policy/license change. +const ( + toolCallRejectedByUserResult = "Tool call rejected by user" + toolCallPolicyDeniedResult = "Tool call was not executed because it is no longer permitted by policy or license" +) + // isRemoteMCPLicensed reports whether the server license covers remote MCP // servers. A nil license checker fails closed. func (c *Conversations) isRemoteMCPLicensed() bool { @@ -278,6 +286,20 @@ func (c *Conversations) HandleToolCall(ctx context.Context, userID string, post IsError: false, }) } + case block.WouldAutoExecute: + // Fresh policy/license check failed. Keep StatusRejected and the + // WouldAutoExecute marker so HasRejectedToolCall does not treat + // this as a human rejection, but record an accurate result so + // the follow-up cannot blame the user for an admin change. + rejectedToolNames = append(rejectedToolNames, block.Name) + block.Status = conversation.StatusRejected + resolvedAny = true + toolResults = append(toolResults, toolrunner.ToolResult{ + ToolCallID: block.ID, + Name: block.Name, + Result: toolCallPolicyDeniedResult, + IsError: true, + }) default: rejectedToolNames = append(rejectedToolNames, block.Name) block.Status = conversation.StatusRejected @@ -287,7 +309,7 @@ func (c *Conversations) HandleToolCall(ctx context.Context, userID string, post toolResults = append(toolResults, toolrunner.ToolResult{ ToolCallID: block.ID, Name: block.Name, - Result: "Tool call rejected by user", + Result: toolCallRejectedByUserResult, IsError: true, }) } @@ -330,8 +352,9 @@ func (c *Conversations) HandleToolCall(ctx context.Context, userID string, post } // Interaction results (answered or skipped) are user-authored, so they // are terminal and shared with no separate share/keep-private step. - // Rejected results share only the canned rejection reason; tool_use - // arguments stay unshared so they are not paraphrased into a channel reply. + // Rejected results (user rejection or policy/license denial) share only + // the canned reason; tool_use arguments stay unshared so they are not + // paraphrased into a channel reply. rejected := toolUseStatusByID[tr.ToolCallID] == conversation.StatusRejected terminal := isDM || interactionByID[tr.ToolCallID] || autoExecutedNow[tr.ToolCallID] || rejected rb := conversation.ContentBlock{ diff --git a/conversations/tool_approval_license_test.go b/conversations/tool_approval_license_test.go index cfebcb646..c971ebcd8 100644 --- a/conversations/tool_approval_license_test.go +++ b/conversations/tool_approval_license_test.go @@ -196,7 +196,7 @@ func TestHandleToolCallLicenseGate(t *testing.T) { licensed: false, accept: false, wantStatus: conversation.StatusRejected, - wantResult: "Tool call rejected by user", + wantResult: toolCallRejectedByUserResult, wantFollowUp: true, wantRejectionGuidance: true, }, diff --git a/conversations/tool_rejection_followup_test.go b/conversations/tool_rejection_followup_test.go index 7bb4334e8..3c9469b61 100644 --- a/conversations/tool_rejection_followup_test.go +++ b/conversations/tool_rejection_followup_test.go @@ -64,10 +64,10 @@ func TestHandleToolCallRejectionFollowsUp(t *testing.T) { blocks: []conversation.ContentBlock{pendingJira("tool-use-1", true)}, wantFollowUp: true, wantGuidance: true, - wantRequestHas: []string{llm.ToolRejectionUserMessage, "Tool call rejected by user"}, + wantRequestHas: []string{llm.ToolRejectionUserMessage, toolCallRejectedByUserResult}, wantToolUseShared: []bool{true}, wantResultShared: []bool{true}, - wantResultContents: []string{"Tool call rejected by user"}, + wantResultContents: []string{toolCallRejectedByUserResult}, }, { name: "lone channel rejection continues with visible reason and private args", @@ -75,11 +75,11 @@ func TestHandleToolCallRejectionFollowsUp(t *testing.T) { blocks: []conversation.ContentBlock{pendingJira("tool-use-1", false)}, wantFollowUp: true, wantGuidance: true, - wantRequestHas: []string{llm.ToolRejectionUserMessage, "Tool call rejected by user"}, + wantRequestHas: []string{llm.ToolRejectionUserMessage, toolCallRejectedByUserResult}, wantRequestOmits: []string{plantedRejectionArg}, wantToolUseShared: []bool{false}, wantResultShared: []bool{true}, - wantResultContents: []string{"Tool call rejected by user"}, + wantResultContents: []string{toolCallRejectedByUserResult}, }, { name: "mixed DM accept and reject includes rejection guidance", @@ -98,10 +98,10 @@ func TestHandleToolCallRejectionFollowsUp(t *testing.T) { acceptedIDs: []string{"tool-use-1"}, wantFollowUp: true, wantGuidance: true, - wantRequestHas: []string{llm.ToolRejectionUserMessage, "Tool call rejected by user", "restored-result"}, + wantRequestHas: []string{llm.ToolRejectionUserMessage, toolCallRejectedByUserResult, "restored-result"}, wantToolUseShared: []bool{true, true}, wantResultShared: []bool{true, true}, - wantResultContents: []string{"restored-result", "Tool call rejected by user"}, + wantResultContents: []string{"restored-result", toolCallRejectedByUserResult}, }, { name: "execution error continues without rejection guidance", @@ -117,6 +117,26 @@ func TestHandleToolCallRejectionFollowsUp(t *testing.T) { wantResultShared: []bool{true}, wantResultContents: []string{"jira unavailable"}, }, + { + name: "policy-denied auto-exec continues without blaming the user", + channel: dmChannel, + blocks: []conversation.ContentBlock{{ + Type: conversation.BlockTypeToolUse, + ID: "tool-use-1", + Name: "jira__get_issue", + Input: json.RawMessage(`{"issue_key":"` + plantedRejectionArg + `"}`), + Status: conversation.StatusPending, + WouldAutoExecute: true, + Shared: conversation.BoolPtr(true), + }}, + wantFollowUp: true, + wantGuidance: false, + wantRequestHas: []string{toolCallPolicyDeniedResult}, + wantRequestOmits: []string{llm.ToolRejectionUserMessage, toolCallRejectedByUserResult}, + wantToolUseShared: []bool{true}, + wantResultShared: []bool{true}, + wantResultContents: []string{toolCallPolicyDeniedResult}, + }, { name: "skipped AskUserQuestion continues without rejection guidance", channel: dmChannel, @@ -269,7 +289,7 @@ func TestHandleToolCallMixedChannelRejectionGuidanceAfterShare(t *testing.T) { assert.False(t, *resultBlocks[0].Shared) assert.NotNil(t, resultBlocks[1].DecidedAt) assert.True(t, *resultBlocks[1].Shared) - assert.Equal(t, "Tool call rejected by user", resultBlocks[1].Content) + assert.Equal(t, toolCallRejectedByUserResult, resultBlocks[1].Content) require.NoError(t, c.HandleToolResult(context.Background(), "user-id", approvalPost, channel, []string{"tool-use-1"})) streamingService.waitForStreaming() @@ -277,7 +297,7 @@ func TestHandleToolCallMixedChannelRejectionGuidanceAfterShare(t *testing.T) { require.Len(t, lm.requests, 1) requestText := completionRequestText(lm.requests[0]) requireRejectionGuidanceIsFinalUserPost(t, lm.requests[0].Posts) - assert.Contains(t, requestText, "Tool call rejected by user") + assert.Contains(t, requestText, toolCallRejectedByUserResult) assert.Contains(t, requestText, "restored-result") assert.NotContains(t, requestText, plantedRejectionArg) } @@ -297,7 +317,7 @@ func TestHandleToolResultRejectedOnlyDoesNotFollowUp(t *testing.T) { resultBlocks := []conversation.ContentBlock{{ Type: conversation.BlockTypeToolResult, ToolUseID: "tool-use-1", - Content: "Tool call rejected by user", + Content: toolCallRejectedByUserResult, Status: conversation.StatusError, Shared: conversation.BoolPtr(true), }} @@ -345,7 +365,7 @@ func TestStreamToolFollowUpLatestToolBatchGuidance(t *testing.T) { humanRejectResult := conversation.ContentBlock{ Type: conversation.BlockTypeToolResult, ToolUseID: "human-1", - Content: "Tool call rejected by user", + Content: toolCallRejectedByUserResult, Status: conversation.StatusError, Shared: conversation.BoolPtr(true), } @@ -406,7 +426,7 @@ func TestStreamToolFollowUpLatestToolBatchGuidance(t *testing.T) { policyDeniedResult := conversation.ContentBlock{ Type: conversation.BlockTypeToolResult, ToolUseID: "auto-1", - Content: "Tool call rejected by user", + Content: toolCallPolicyDeniedResult, Status: conversation.StatusError, Shared: conversation.BoolPtr(true), } From 1e63c07602af00ca50d8568ae26b792de506c663 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Tue, 1 Sep 2026 13:47:04 +0000 Subject: [PATCH 4/5] Drop removed ToolCall.Schema from conversion test Master removed Schema from llm.ToolCall; keep Title, Description, and rejection signals in the identity-metadata test. Co-authored-by: Nick Misasi --- conversation/convert_test.go | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/conversation/convert_test.go b/conversation/convert_test.go index 19cf44c20..45f2d8e00 100644 --- a/conversation/convert_test.go +++ b/conversation/convert_test.go @@ -242,18 +242,12 @@ func TestPostToBlocksPreservesToolIdentityMetadata(t *testing.T) { post := llm.Post{ Role: llm.PostRoleBot, ToolUse: []llm.ToolCall{{ - ID: "tc1", - Name: "jira__get_issue", - Description: "Get a Jira issue", - Title: "Get Issue", - ServerOrigin: "https://jira.example.com", - Arguments: json.RawMessage(`{"key":"MM-1"}`), - Schema: map[string]any{ - "type": "object", - "properties": map[string]any{ - "key": map[string]any{"type": "string"}, - }, - }, + ID: "tc1", + Name: "jira__get_issue", + Description: "Get a Jira issue", + Title: "Get Issue", + ServerOrigin: "https://jira.example.com", + Arguments: json.RawMessage(`{"key":"MM-1"}`), MCPBareName: "get_issue", Status: llm.ToolCallStatusPending, UserInteraction: llm.UserInteractionSelect, From cf56c71aa5cb44a6951670d254041b0d9fed18dc Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Fri, 11 Sep 2026 14:11:42 +0000 Subject: [PATCH 5/5] Fix conversations tests after ABAC bots.New signature change Restore the license-gate follow-up LLM/streaming stubs dropped in the master merge, and pass the passthrough access checker into bots.New. Co-authored-by: mattermost-code --- conversations/tool_approval_license_test.go | 4 +++- conversations/tool_rejection_followup_test.go | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/conversations/tool_approval_license_test.go b/conversations/tool_approval_license_test.go index a66d002c3..5a20018b2 100644 --- a/conversations/tool_approval_license_test.go +++ b/conversations/tool_approval_license_test.go @@ -118,7 +118,9 @@ func toolLicenseConversations(t *testing.T, convStore *loadedStateFlowStore, lic pluginAPI := pluginapi.NewClient(mockAPI, nil) licenseChecker := toolLicenseChecker(t, licensed) botsService := bots.New(mockAPI, pluginAPI, licenseChecker, nil, nil, newPassthroughAccessChecker(), &http.Client{}, nil) - botsService.SetBotsForTesting([]*bots.Bot{toolLicenseTestBot()}) + lm := &loadedStateLLM{} + streamingService := &loadedStateStreamingService{} + botsService.SetBotsForTesting([]*bots.Bot{toolLicenseTestBot(lm)}) mmClient := mocks.NewMockClient(t) mmClient.On("LogDebug", mock.Anything, mock.Anything).Maybe().Return() diff --git a/conversations/tool_rejection_followup_test.go b/conversations/tool_rejection_followup_test.go index 85b2e61a1..384502566 100644 --- a/conversations/tool_rejection_followup_test.go +++ b/conversations/tool_rejection_followup_test.go @@ -577,7 +577,7 @@ func newRejectionFollowUpConversations( mockAPI := &plugintest.API{} pluginAPI := pluginapi.NewClient(mockAPI, nil) licenseChecker := enterprise.NewLicenseChecker(pluginAPI) - botsService := bots.New(mockAPI, pluginAPI, licenseChecker, nil, nil, &http.Client{}, nil) + botsService := bots.New(mockAPI, pluginAPI, licenseChecker, nil, nil, newPassthroughAccessChecker(), &http.Client{}, nil) botsService.SetBotsForTesting([]*bots.Bot{loadedStateBot(lm)}) mmClient := mocks.NewMockClient(t)