Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Migrate notifications toolset to modelcontextprotocol/go-sdk#1449

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged

Conversation

Copy link
Contributor

CopilotAI commentedNov 20, 2025•
edited
Loading

Closes: Part of#1428

Migrates 6 notification tools frommark3labs/mcp-go tomodelcontextprotocol/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, err →result, _, err
  • Removed schema property assertions (InputSchema opaque in new SDK)

Example

// Beforereturnmcp.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")returnmcp.NewToolResultText(result),nil}// Aftertool:= 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,argsmap[string]any) (*mcp.CallToolResult,any,error) {filter,err:=OptionalParam[string](args,"filter")returnutils.NewToolResultText(result),nil,nil})

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

Original prompt

Migrate thenotifications 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 our2 minute survey.

Co-authored-by: omgitsads <4619+omgitsads@users.noreply.github.com>
CopilotAI changed the title[WIP] Migrate notifications toolset to modelcontextprotocol/go-sdkMigrate notifications toolset to modelcontextprotocol/go-sdkNov 20, 2025
CopilotAI requested a review fromomgitsadsNovember 20, 2025 10:37
Copilot finished work on behalf ofomgitsadsNovember 20, 2025 10:37
@LuluBeatsonLuluBeatson self-requested a reviewNovember 20, 2025 18:33
"type":"object",
"required": [
"threadID",
"state"
Copy link
Contributor

@LuluBeatsonLuluBeatsonNov 20, 2025•
edited
Loading

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

imageimageimage

⚠️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.

imageimage

@LuluBeatsonLuluBeatson marked this pull request as ready for reviewNovember 20, 2025 19:10
@LuluBeatsonLuluBeatson requested a review froma team as acode ownerNovember 20, 2025 19:10
CopilotAI review requested due to automatic review settingsNovember 20, 2025 19:10
Copilot finished reviewing on behalf ofLuluBeatsonNovember 20, 2025 19:13
Copy link
Contributor

CopilotAI 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 frommark3labs/mcp-go tomodelcontextprotocol/go-sdk as part of the broader SDK migration effort (#1428). The migration updates tool definitions to usejsonschema.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.) tojsonschema.Schema structs
  • Parameter extraction now usesargs map[string]any instead ofrequest
  • Result helpers updated frommcp.NewToolResultText() toutils.NewToolResultText()

Reviewed Changes

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

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

@omgitsadsomgitsads merged commitee72841 intoomgitsads/go-sdkNov 24, 2025
13 of 14 checks passed
@omgitsadsomgitsads deleted the copilot/migrate-notifications-toolset branchNovember 24, 2025 10:53
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

Copilot code reviewCopilotCopilot left review comments

@LuluBeatsonLuluBeatsonLuluBeatson approved these changes

@omgitsadsomgitsadsAwaiting requested review from omgitsads

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@LuluBeatson@omgitsads
[8]ページ先頭
©2009-2025 Movatter.jp