Fix high-severity SonarQube issues (S1192, S3776) in notifications.go

[eashansinha]

Summary

Resolves the high-severity SonarQube findings in pkg/github/notifications.go:

• S1192 (duplicated string literals): repeated literals are now package-level constants.
• S3776 (cognitive complexity): every tool handler is now well under the complexity threshold.

No behavior or error-message changes — purely a refactor. All existing tests pass.

What changed

S1192 — constants for repeated literals:

• Error formats: errFailedToGetClient, errFailedToReadBody, errFailedToMarshal.
• Parameter names: paramOwner, paramRepo, paramAction, paramNotificationID, paramSince, paramBefore, paramLastReadAt.

S3776 — reduce cognitive complexity:
The tool-factory functions previously inlined all logic inside the returned handler closure, which inflated cognitive complexity (nested-function nesting penalty). Each handler body is now a named package-level function (e.g. listNotificationsHandler), and shared logic is extracted into helpers:

• marshalToolResult(v) — JSON-marshal + text result.
• responseErrorResult(resp, message) — read body + error result on unexpected status.
• parseOptionalTimestamp(value, field) — optional RFC3339 parsing.
• parseListNotificationsParams(request) — extracts the list_notifications params.
• setThreadSubscription(...) / setRepositorySubscription(...) — the ignore/watch/delete switch.

Cognitive complexity (gocognit) before → after:

ListNotifications                          37 -> 11 (listNotificationsHandler)
MarkAllNotificationsRead                   26 -> 12
ManageRepositoryNotificationSubscription   22 -> 11
DismissNotification                        19 ->  9
ManageNotificationSubscription             17 ->  8

All handlers are now ≤ 12.

Verification

• go build ./..., go vet ./pkg/github/ — clean
• go test -race ./... — pass
• golangci-lint run ./pkg/github/ (v2.1) — 0 issues

Tradeoffs / Alternatives

• The handler logic could have stayed in the closures with only the switch/marshal extracted, but the closure nesting penalty kept complexity near the limit; moving bodies to named functions removes that penalty cleanly.
• Constants for short param names (owner, repo) are file-local to this change to match SonarQube's per-file duplication scope; broader codebase adoption is out of scope here.

Link to Devin session: https://app.devin.ai/sessions/ad357e445c824ab194f9ab6c7a782484
Requested by: @eashansinha

---

Devin Review

Status	Commit
🟢 Reviewed	1dfcc2b

[devin-ai-integration]

Original prompt from Eashan

Fix the following high severity SonarQube issues in COG-GTM/github-mcp-server:
Repository: COG-GTM/github-mcp-server File: pkg/github/notifications.go Issues (8 total):

Fix these issues by:
For S1192 (duplicated strings): Define constants at the package level for repeated string literals
For S3776 (cognitive complexity): Refactor complex methods by extracting helper functions
Run tests after fixes to ensure nothing breaks.

[devin-ai-integration]

🤖 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, CI, and merge conflict monitoring

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[devin-ai-integration]

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no potential bugs to report.

View in Devin Review to see 3 additional findings.

[staging-devin-ai-integration]

Devin Review found 1 potential issue.

View 2 additional findings in Devin Review.

Debug

Playground

[staging-devin-ai-integration]

🟡 Zero RFC3339 timestamps are treated as omitted

parseOptionalTimestamp returns the zero time.Time both when a timestamp argument is omitted and when the caller provides the valid RFC3339 value 0001-01-01T00:00:00Z; the handlers then use IsZero() as the “not provided” check. For mark_all_notifications_read, that means an explicit lastReadAt of 0001-01-01T00:00:00Z is replaced with time.Now(), so the tool marks current notifications as read instead of honoring the supplied cutoff. The same sentinel pattern also causes explicit zero values for since/before in list_notifications to be silently dropped at pkg/github/notifications.go:165-178.

Prompt for agents

The notification timestamp parsing now uses a zero time as the sentinel for an omitted argument, but a parsed RFC3339 zero timestamp is also a zero time. Update the timestamp handling in pkg/github/notifications.go so callers distinguish “argument not provided” from “argument provided and parsed to time.Time{}”. For markAllNotificationsReadHandler, only default to time.Now() when lastReadAt is the empty string. For listNotificationsHandler, set opts.Since/opts.Before whenever the corresponding raw parameter is non-empty and parsing succeeded, even if the parsed time is IsZero(). Consider changing parseOptionalTimestamp to return an additional boolean indicating whether a value was provided.

---

Was this helpful? React with 👍 or 👎 to provide feedback.

Debug

Playground