Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 56 additions & 0 deletions server/regular_tool_as_task_panic_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
package server

import (
"context"
"testing"
"time"

"github.com/mark3labs/mcp-go/mcp"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)

// TestExecuteRegularToolAsTask_PanicRecovery mirrors
// TestExecuteTaskTool_PanicRecovery (added in #880) for the sibling code
// path: a regular ServerTool with TaskSupportOptional invoked asynchronously
// via the hybrid task mode. That fix covered executeTaskTool but did not
// touch executeRegularToolAsTask, which still ran without panic recovery --
// an unrecovered panic in this goroutine crashes the whole server process.
func TestExecuteRegularToolAsTask_PanicRecovery(t *testing.T) {
s := NewMCPServer("test", "1.0.0")

regularTool := ServerTool{
Tool: mcp.Tool{
Name: "panic-regular-tool",
Description: "A regular tool that panics when run as a task",
},
Handler: func(ctx context.Context, request mcp.CallToolRequest) (*mcp.CallToolResult, error) {
panic("deliberate panic in regular tool handler run as a task")
},
}

ctx := t.Context()
taskID := "test-regular-panic-task"
entry, err := s.createTask(ctx, taskID, "panic-regular-tool", nil, nil)
require.NoError(t, err)

request := mcp.CallToolRequest{}
request.Params.Name = "panic-regular-tool"

// Execute in a goroutine, same as the production hybrid-mode path.
go s.executeRegularToolAsTask(ctx, entry, regularTool, request)

select {
case <-entry.done:
// Task completed without crashing the process.
case <-time.After(5 * time.Second):
t.Fatal("task did not complete within timeout; panic recovery may have failed")
}

s.tasksMu.RLock()
assert.True(t, entry.completed)
assert.Equal(t, mcp.TaskStatusFailed, entry.task.Status)
assert.Contains(t, entry.task.StatusMessage, "panic in task tool handler")
assert.Contains(t, entry.task.StatusMessage, "deliberate panic in regular tool handler run as a task")
s.tasksMu.RUnlock()
}
6 changes: 6 additions & 0 deletions server/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -2222,6 +2222,12 @@ func (s *MCPServer) executeRegularToolAsTask(
regularTool ServerTool,
request mcp.CallToolRequest,
) {
defer func() {
if r := recover(); r != nil {
s.completeTask(entry, nil, fmt.Errorf("panic in task tool handler %s: %v", request.Params.Name, r))
}
Comment on lines +2225 to +2228

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Sanitize panic details across the task error contract.

Raw panic values can leak sensitive handler or implementation details through task status notifications.

  • server/server.go#L2225-L2228: record detailed panic diagnostics server-side, but complete the task with a generic client-safe error.
  • server/regular_tool_as_task_panic_test.go#L52-L54: assert the sanitized status message rather than the raw panic text.
📍 Affects 2 files
  • server/server.go#L2225-L2228 (this comment)
  • server/regular_tool_as_task_panic_test.go#L52-L54
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@server/server.go` around lines 2225 - 2228, Sanitize panic details in the
task error contract: in server/server.go lines 2225-2228, retain detailed panic
diagnostics in server-side logging or recovery handling but pass a generic
client-safe error to completeTask. Update
server/regular_tool_as_task_panic_test.go lines 52-54 to assert the generic
sanitized status message instead of raw panic text.

}()

// Create cancellable context for this task execution
taskCtx, cancel := context.WithCancel(ctx)
defer cancel()
Expand Down
Loading