Skip to content

refactor(notifications): migrate notifications.go to NewTool pattern #1592

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.

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom
Open
SamMorrowDrums wants to merge 2 commits into
base: SamMorrowDrums/server-tool-refactor-gists
Choose a base branch
from
Open

refactor(notifications): migrate notifications.go to NewTool pattern #1592

SamMorrowDrums wants to merge 2 commits into from
+776 −732

Conversation

@SamMorrowDrums
Copy link
Collaborator

@SamMorrowDrums SamMorrowDrums commented Dec 13, 2025

Summary

Migrates all notification tools to the new NewTool pattern with ToolDependencies injection.

Part of stacked PRs for ServerTool refactoring:

  1. refactor: Separate ServerTool with HandlerFunc pattern and ToolDependencies #1589 - Foundation + search.go
  2. Migrate context_tools to new ServerTool pattern #1590 - context_tools.go
  3. refactor(gists): migrate gists.go to NewTool pattern #1591 - gists.go ⬅️ parent
  4. This PR - notifications.go

Changes

Converts all 6 notification tools:

  • ListNotifications

  • GetNotificationDetails

  • DismissNotification

  • MarkAllNotificationsRead

  • ManageNotificationSubscription

  • ManageRepositoryNotificationSubscription

  • Remove getClient parameter from function signatures

  • Functions now only take t translations.TranslationHelperFunc and return toolsets.ServerTool

  • Use deps.GetClient(ctx) inside handlers to get the GitHub client

  • Update all tests to use serverTool.Handler(deps) pattern

Testing

  • All existing notification tests updated to new pattern
  • script/lint passes
  • script/test passes

Co-authored-by: Adam Holt oholt@github.com

@SamMorrowDrums
refactor(gists): migrate gists.go to NewTool pattern
af97440 
Convert all gist tools (ListGists, GetGist, CreateGist, UpdateGist)
to use the new NewTool helper with ToolDependencies injection.

- Remove getClient parameter from function signatures
- Use deps.GetClient(ctx) inside handlers
- Standardize error handling with utils.NewToolResultErrorFromErr()
- Update all tests to use serverTool.Handler(deps) pattern

Co-authored-by: Adam Holt <oholt@github.com>
@SamMorrowDrums SamMorrowDrums requested a review from a team as a code owner December 13, 2025 13:05
Copilot AI review requested due to automatic review settings December 13, 2025 13:05
Copilot AI reviewed Dec 13, 2025
View reviewed changes
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 all 6 notification-related tools from the legacy pattern to the new NewTool pattern with ToolDependencies injection, continuing the refactoring effort across the codebase.

Key Changes:

  • Removed getClient parameter from all notification tool function signatures
  • Updated tools to use deps.GetClient(ctx) inside handlers instead of passed-in client function
  • Converted all test cases to use the new serverTool.Handler(deps) pattern
  • Ensured consistent error handling with nil as the third return parameter

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.

File Description
pkg/github/tools.go Updated toolset registration to use new pattern, removing toolsets.NewServerToolLegacy wrapper calls for all 6 notification tools
pkg/github/notifications.go Migrated all 6 notification tools (ListNotifications, GetNotificationDetails, DismissNotification, MarkAllNotificationsRead, ManageNotificationSubscription, ManageRepositoryNotificationSubscription) to return toolsets.ServerTool and accept dependencies via closure
pkg/github/notifications_test.go Updated all test functions to use serverTool.Handler(deps) pattern, but introduced duplicate error checks in multiple test cases

pkg/github/notifications_test.go
if tc.expectError {
// The tool returns a ToolResultError with a specific message
require.NoError(t, err)
require.NotNil(t, result)
Copy link

Copilot AI Dec 13, 2025

Choose a reason for hiding this comment

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

Duplicate error check. The require.NoError(t, err) is already performed at line 573 before entering this block. This redundant check should be removed.

Copilot uses AI. Check for mistakes.
pkg/github/notifications_test.go
Comment on lines +698 to 699
require.NoError(t, err)
if tc.expectError {
Copy link

Copilot AI Dec 13, 2025

Choose a reason for hiding this comment

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

Duplicate error check. The require.NoError(t, err) is already performed at line 698 before entering this block. This redundant check should be removed.

Suggested change
require.NoError(t, err)
if tc.expectError {
if tc.expectError {
require.Error(t, err)

Copilot uses AI. Check for mistakes.
pkg/github/notifications_test.go
Comment on lines +782 to 783
require.NoError(t, err)
if tc.expectError {
Copy link

Copilot AI Dec 13, 2025

Choose a reason for hiding this comment

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

Duplicate error check. The require.NoError(t, err) is already performed at line 782 before entering this block. This redundant check should be removed.

Suggested change
require.NoError(t, err)
if tc.expectError {
if tc.expectError {
require.Error(t, err)

Copilot uses AI. Check for mistakes.
pkg/github/notifications_test.go

require.NoError(t, err)
if tc.expectError {
require.NoError(t, err)
Copy link

Copilot AI Dec 13, 2025

Choose a reason for hiding this comment

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

Duplicate error check. The require.NoError(t, err) is already performed at line 268 before entering this block. This redundant check should be removed.

Suggested change
require.NoError(t, err)

Copilot uses AI. Check for mistakes.
pkg/github/notifications_test.go
Comment on lines +431 to 432
require.NoError(t, err)
if tc.expectError {
Copy link

Copilot AI Dec 13, 2025

Choose a reason for hiding this comment

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

Duplicate error check. The require.NoError(t, err) is already performed at line 431 before entering this block. This redundant check should be removed.

Suggested change
require.NoError(t, err)
if tc.expectError {
if tc.expectError {
require.Error(t, err)

Copilot uses AI. Check for mistakes.
This was referenced Dec 13, 2025
Open
Open
Open
@SamMorrowDrums @omgitsads
refactor(notifications): migrate notifications.go to NewTool pattern
5e904e8 
Convert all notification tools to use the new NewTool helper with
ToolDependencies injection.

Co-authored-by: Adam Holt <omgitsads@users.noreply.github.com>
@SamMorrowDrums SamMorrowDrums force-pushed the SamMorrowDrums/server-tool-refactor-notifications branch from 34cf61e to 5e904e8 Compare December 13, 2025 14:17
@SamMorrowDrums SamMorrowDrums force-pushed the SamMorrowDrums/server-tool-refactor-gists branch from af97440 to 820ce1e Compare December 13, 2025 14:18
This was referenced Dec 13, 2025
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@SamMorrowDrums