Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Nov 20, 2025

Closes: Part of #1428

Migrates 6 notification tools from mark3labs/mcp-go to modelcontextprotocol/go-sdk:

  • list_notifications
  • dismiss_notification
  • mark_all_notifications_read
  • get_notification_details
  • manage_notification_subscription
  • manage_repository_notification_subscription

Changes

Tool Implementation

  • Updated return type: (mcp.Tool, server.ToolHandlerFunc)(mcp.Tool, mcp.ToolHandlerFor[map[string]any, any])
  • Handler signature: func(context.Context, mcp.CallToolRequest) (*mcp.CallToolResult, error)func(context.Context, *mcp.CallToolRequest, map[string]any) (*mcp.CallToolResult, any, error)
  • Schema conversion: DSL-based (mcp.NewTool(), mcp.WithString()) → jsonschema.Schema structs
  • Parameter extraction: OptionalParam[T](request, "key")OptionalParam[T](args, "key")
  • Result helpers: mcp.NewToolResultText()utils.NewToolResultText()

Test Updates

  • Handler invocation: handler(ctx, request)handler(ctx, &request, args)
  • Result handling: result, errresult, _, err
  • Removed schema property assertions (InputSchema opaque in new SDK)

Example

// Before
return mcp.NewTool("list_notifications",
    mcp.WithString("filter", mcp.Enum("default", "include_read_notifications")),
    // ...
),
func(ctx context.Context, request mcp.CallToolRequest) (*mcp.CallToolResult, error) {
    filter, err := OptionalParam[string](request, "filter")
    return mcp.NewToolResultText(result), nil
}

// After
tool := mcp.Tool{
    Name: "list_notifications",
    InputSchema: &jsonschema.Schema{
        Properties: map[string]*jsonschema.Schema{
            "filter": {Type: "string", Enum: []any{"default", "include_read_notifications"}},
        },
    },
}
handler := mcp.ToolHandlerFor[map[string]any, any](func(ctx context.Context, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) {
    filter, err := OptionalParam[string](args, "filter")
    return utils.NewToolResultText(result), nil, nil
})

Toolsnaps updated to reflect new schema format while maintaining logical equivalence.

Original prompt

Migrate the notifications toolset to modelcontextprotocol/go-sdk.

This will be merged into the parent PR #1428, so reference that PR in the description to track the migration

Custom agent used: go-sdk-tool-migrator
Agent specializing in migrating MCP tools from mark3labs/mcp-go to modelcontextprotocol/go-sdk


💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

Co-authored-by: omgitsads <4619+omgitsads@users.noreply.github.com>
Copilot AI changed the title [WIP] Migrate notifications toolset to modelcontextprotocol/go-sdk Migrate notifications toolset to modelcontextprotocol/go-sdk Nov 20, 2025
Copilot AI requested a review from omgitsads November 20, 2025 10:37
Copilot finished work on behalf of omgitsads November 20, 2025 10:37
@LuluBeatson LuluBeatson self-requested a review November 20, 2025 18:33
"type": "object",
"required": [
"threadID",
"state"
Copy link
Contributor

@LuluBeatson LuluBeatson Nov 20, 2025

Choose a reason for hiding this comment

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

state was not required before migration. But actually, the tool handler requires it.

This is an acceptable change in the tool schema.

@LuluBeatson
Copy link
Contributor

image image image

⚠️ mark_all_notifications_read returns 202 (Accepted) or 205 (Reset Content)

https://docs.github.com/en/rest/activity/notifications?apiVersion=2022-11-28#mark-repository-notifications-as-read--status-codes

At the moment a 202 creates a tool error. Re-running the tool a few moments later creates a 205 successful tool response. I think we need to consider whether this async API endpoint returning 202 should be considered a tool success.

This behaviour is the same as before migration. So will not be fixed in this PR.

image image

@LuluBeatson LuluBeatson marked this pull request as ready for review November 20, 2025 19:10
@LuluBeatson LuluBeatson requested a review from a team as a code owner November 20, 2025 19:10
Copilot AI review requested due to automatic review settings November 20, 2025 19:10
Copilot finished reviewing on behalf of LuluBeatson November 20, 2025 19:13
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR migrates 6 notification tools from mark3labs/mcp-go to modelcontextprotocol/go-sdk as part of the broader SDK migration effort (#1428). The migration updates tool definitions to use jsonschema.Schema structs instead of DSL-based builders, updates handler signatures to match the new SDK's three-return-value pattern, and adjusts parameter extraction and result creation to use the new SDK's interfaces.

Key Changes

  • Tool return types changed from (mcp.Tool, server.ToolHandlerFunc) to (mcp.Tool, mcp.ToolHandlerFor[map[string]any, any])
  • Schema definitions converted from DSL (mcp.WithString(), etc.) to jsonschema.Schema structs
  • Parameter extraction now uses args map[string]any instead of request
  • Result helpers updated from mcp.NewToolResultText() to utils.NewToolResultText()

Reviewed Changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 7 comments.

File Description
pkg/github/notifications.go Migrated 6 notification tool implementations to new SDK, including schema definitions and handler logic
pkg/github/notifications_test.go Updated test invocations for new handler signatures (3 return values) and schema assertions to use *jsonschema.Schema
pkg/github/tools.go Uncommented notifications toolset registration to re-enable these tools
pkg/github/__toolsnaps__/*.snap Updated toolsnaps to reflect new schema serialization format (field ordering, optional false values omitted)

Enum: []any{"read", "done"},
},
},
Required: []string{"threadID", "state"},
Copy link

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

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

The state parameter is now marked as required, but it was optional in the original implementation (the old toolsnap shows only "threadID" in the required array). This changes the API behavior - users were previously able to omit the state parameter.

If making state required is intentional, this should be called out in the PR description as a behavior change. Otherwise, state should remain optional to maintain backward compatibility.

Copilot uses AI. Check for mistakes.
return mcp.NewToolResultError(err.Error()), nil
}
func ListNotifications(getClient GetClientFn, t translations.TranslationHelperFunc) (mcp.Tool, mcp.ToolHandlerFor[map[string]any, any]) {
tool := mcp.Tool{
Copy link

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

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

This definition of tool is never used.

Copilot uses AI. Check for mistakes.
return nil, fmt.Errorf("failed to get GitHub client: %w", err)
}
func DismissNotification(getclient GetClientFn, t translations.TranslationHelperFunc) (mcp.Tool, mcp.ToolHandlerFor[map[string]any, any]) {
tool := mcp.Tool{
Copy link

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

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

This definition of tool is never used.

Copilot uses AI. Check for mistakes.
return nil, fmt.Errorf("failed to get GitHub client: %w", err)
}
func MarkAllNotificationsRead(getClient GetClientFn, t translations.TranslationHelperFunc) (mcp.Tool, mcp.ToolHandlerFor[map[string]any, any]) {
tool := mcp.Tool{
Copy link

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

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

This definition of tool is never used.

Copilot uses AI. Check for mistakes.
return mcp.NewToolResultError(err.Error()), nil
}
func GetNotificationDetails(getClient GetClientFn, t translations.TranslationHelperFunc) (mcp.Tool, mcp.ToolHandlerFor[map[string]any, any]) {
tool := mcp.Tool{
Copy link

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

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

This definition of tool is never used.

Copilot uses AI. Check for mistakes.
return nil, fmt.Errorf("failed to get GitHub client: %w", err)
}
func ManageNotificationSubscription(getClient GetClientFn, t translations.TranslationHelperFunc) (mcp.Tool, mcp.ToolHandlerFor[map[string]any, any]) {
tool := mcp.Tool{
Copy link

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

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

This definition of tool is never used.

Copilot uses AI. Check for mistakes.
return nil, fmt.Errorf("failed to get GitHub client: %w", err)
}
func ManageRepositoryNotificationSubscription(getClient GetClientFn, t translations.TranslationHelperFunc) (mcp.Tool, mcp.ToolHandlerFor[map[string]any, any]) {
tool := mcp.Tool{
Copy link

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

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

This definition of tool is never used.

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants