Skip to content

feat: add GitHub notifications tools for managing user notifications - #225

Merged
williammartin merged 2 commits into
mainfrom
notifications-tooling
May 23, 2025
Merged

feat: add GitHub notifications tools for managing user notifications#225
williammartin merged 2 commits into
mainfrom
notifications-tooling

Conversation

@sridharavinash

@sridharavinashsridharavinash commented Apr 11, 2025

Copy link
Copy Markdown
Contributor

GitHub Notifications Tooling

This PR adds support for managing GitHub notifications

Features Added

New Tools

  • get_notifications: Retrieve a list of notifications for the authenticated GitHub user with filtering options for read/unread status, participation, and time ranges
  • mark_notification_read: Mark a specific notification thread as read
  • mark_notification_done: Mark a specific notification thread as done
  • mark_all_notifications_read: Mark all notifications as read with an optional timestamp
  • get_notification_thread: Fetch details of a specific notification thread

Technical Implementation

  • Implements proper error handling and status code validation
  • Provides RFC3339/ISO8601 timestamp parsing for time-related parameters
  • Integrates with the existing GitHub API client infrastructure
  • Respects read-only mode settings for mutating operations

@sridharavinash
sridharavinash marked this pull request as ready for review April 14, 2025 18:16
CopilotAI review requested due to automatic review settings April 14, 2025 18:16

CopilotAI left a comment

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.

Pull Request Overview

This PR introduces new GitHub notifications management tooling to allow users to retrieve notifications, mark them as read (individually or in bulk), and fetch detailed thread information.

  • Added notifications tools to the server initialization in pkg/github/server.go.
  • Implemented functions in pkg/github/notifications.go for handling notification retrieval and state updates with appropriate error handling and time parsing.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

FileDescription
pkg/github/server.goAdded notifications tools and helper functions for optional params.
pkg/github/notifications.goImplements API integrations for GitHub notifications management.
Comments suppressed due to low confidence (2)

pkg/github/server.go:262

  • The condition unconditionally returns the default value when v is false, which means an explicit false value from the user is ignored. Consider checking for the presence of the parameter instead of evaluating its boolean value directly.
if !v { return d, nil }

pkg/github/notifications.go:130

  • [nitpick] The parameter name 'getclient' is inconsistent with other functions that use 'getClient'. Consider renaming it to 'getClient' for consistency.
func MarkNotificationRead(getclient GetClientFn, t translations.TranslationHelperFunc) (tool mcp.Tool, handler server.ToolHandlerFunc) {

@sridharavinash
sridharavinash requested a review from a team as a code ownerApril 18, 2025 21:13
@SamMorrowDrums
SamMorrowDrumsforce-pushed the notifications-tooling branch 3 times, most recently from 9878a8a to ea51752CompareMay 20, 2025 11:08

CopilotAI left a comment

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.

Pull Request Overview

This PR introduces a new “notifications” toolset for managing GitHub notifications, including unit and end-to-end tests, plus documentation for opting into state-mutating tests.

  • Registers the notifications toolset in pkg/github/tools.go
  • Adds unit tests for each notification tool (pkg/github/notifications_test.go)
  • Implements e2e tests and a README section to skip global-state mutations (e2e/e2e_test.go, e2e/README.md)

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

FileDescription
pkg/github/tools.goRegister new “notifications” toolset
pkg/github/notifications_test.goAdd unit tests for list, get, dismiss, mark-all, and subscription tools
e2e/e2e_test.goAdd e2e tests for notification tools and skip logic
e2e/README.mdDocument environment flag to skip mutation tests
Comments suppressed due to low confidence (5)

pkg/github/notifications_test.go:57

  • Consider adding a unit test for the "watch" action in ManageNotificationSubscription, since currently only "ignore" and "delete" are covered.
func Test_ManageNotificationSubscription(t *testing.T) {

pkg/github/notifications_test.go:114

  • The "watch" action isn’t tested in Test_ManageRepositoryNotificationSubscription; consider adding it to ensure full coverage of subscription states.
func Test_ManageRepositoryNotificationSubscription(t *testing.T) {

pkg/github/notifications_test.go:180

  • Add a success-case test for the "done" state in Test_DismissNotification to cover both supported states.
// Success case: mark as read

pkg/github/notifications_test.go:224

  • Include a unit test for passing the since timestamp into MarkAllNotificationsRead to validate the optional filter parameter.
request := createMCPRequest(map[string]interface{}{})

e2e/e2e_test.go:1552

  • Requiring exactly one notification is brittle—list_notifications can return multiple items. Consider using require.NotEmpty or asserting a minimum length.
require.Len(t, resp.Content, 1, "expected content to have one item")

Comment threadpkg/github/tools.go
Comment threade2e/e2e_test.go Outdated
Comment threade2e/e2e_test.go Outdated
Comment threade2e/e2e_test.go Outdated
Comment threade2e/e2e_test.go Outdated
Comment threadpkg/github/notifications_test.go
Comment threadpkg/github/notifications.go
Comment threadpkg/github/notifications.go
Comment threadpkg/github/notifications.go
Comment threadpkg/github/notifications.go
Comment threadpkg/github/notifications.go
@SamMorrowDrums
SamMorrowDrumsforce-pushed the notifications-tooling branch 2 times, most recently from c09adbd to 7082815CompareMay 23, 2025 10:32

@williammartinwilliammartin left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks!

@williammartin
williammartin merged commit e9f748f into mainMay 23, 2025
@williammartin
williammartin deleted the notifications-tooling branch May 23, 2025 12:00
Sign up for freeto 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.

4 participants

@sridharavinash@williammartin@SamMorrowDrums