Skip to content

Conversation

@devin-ai-integration
Copy link

Summary

Fixes two High severity SonarQube issues in the github-mcp-server codebase:

  1. go:S1192 - String literal duplication: Extracted ErrFailedToGetGitHubClient constant to eliminate 40+ duplicate occurrences of "failed to get GitHub client: %w"
  2. go:S3776 - Cognitive complexity: Refactored ManageNotificationSubscription by extracting executeNotificationSubscriptionAction helper function to reduce complexity from 17 to ≤15

Changes

String Duplication Fix

  • Added ErrFailedToGetGitHubClient constant in pkg/github/server.go
  • Replaced 40+ instances across 9 files:
    • pkg/github/actions.go (14 instances)
    • pkg/github/repositories.go (12 instances)
    • pkg/github/issues.go (7 instances)
    • pkg/github/notifications.go (6 instances)
    • pkg/github/pullrequests.go (multiple instances)
    • pkg/github/search.go (3 instances)
    • pkg/github/code_scanning.go (2 instances)
    • pkg/github/secret_scanning.go (2 instances)
    • pkg/github/repository_resource.go (1 instance)

Note: pkg/github/search_utils.go was intentionally excluded as it uses a dynamic prefix pattern: fmt.Errorf("%s: failed to get GitHub client: %w", errorPrefix, err)

Cognitive Complexity Fix

  • Extracted executeNotificationSubscriptionAction helper function that encapsulates the switch statement for different notification actions (ignore/watch/delete)
  • Simplified ManageNotificationSubscription handler by delegating action execution to the helper
  • Maintained identical functionality while reducing nesting levels

Testing

✅ All lint checks passed (./script/lint - 0 issues)
✅ All tests passed (./script/test)

Review Checklist

Please pay special attention to:

  • Error formatting: Verify that fmt.Errorf(ErrFailedToGetGitHubClient, err) still produces the expected error messages with proper wrapping
  • Invalid action detection: The refactored code uses if apiErr.Error() == fmt.Sprintf("invalid action: %s", action) to distinguish invalid action errors from API errors - confirm this pattern is reliable
  • Response cleanup: Ensure defer func() { _ = resp.Body.Close() }() is still called correctly after the refactoring
  • Cognitive complexity: Confirm the extracted helper function successfully reduces complexity while maintaining the same behavior

Link to Devin run: https://app.devin.ai/sessions/d7851a5770a441d9a7e342cc1c5d63cd
Requested by: Shawn Azman (@ShawnAzman)

- Extract ErrFailedToGetGitHubClient constant to eliminate 40+ string duplications
- Refactor ManageNotificationSubscription to reduce cognitive complexity from 17 to 15
- Extract executeNotificationSubscriptionAction helper function

Fixes go:S1192 and go:S3776 SonarQube issues
@devin-ai-integration
Copy link
Author

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment and CI monitoring

@sonarqubecloud
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
21.2% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants