agentic-forges/forgejo-mcp

Watch

Fork 0

feat(notifications): implement complete notification API #86

Merged

byteflavour merged 0 commits from refs/pull/86/head into main

2026-03-08 10:45:21 +00:00

byteflavour commented

2026-03-08 09:40:36 +00:00

(Migrated from codeberg.org)

Closes #85

The Need

Currently, the forgejo-mcp server exposes the check_notifications tool, which is excellent for passively polling unread notifications. However, for fully autonomous agents that rely heavily on the MCP server to interact with Forgejo, merely reading notifications is insufficient. Without write access to the notification API, agents eventually get stuck or require manual GUI intervention to acknowledge notifications.

The Solution

This PR expands the User/Notifications toolset in forgejo-mcp to expose the full Forgejo SDK notification suite, allowing 100% headless notification management.

Implemented Tools

get_notification_thread (Fetches a single thread by ID)
mark_notification_read (Marks a specific thread as read)
mark_all_notifications_read (Global acknowledgment)
list_repo_notifications (Filters notifications scoped to a specific repository)
mark_repo_notifications_read (Marks all notifications in a specific repository as read)

Implementation Details

Context/Logging: Follows the established log.LogMCPToolStart/LogMCPToolComplete and forgejo.LogAPICall patterns.
TDD: The mockNotificationServer in notification_test.go has been expanded to test all HTTP endpoints and correctly handle empty JSON responses for PUT and PATCH.
Documentation: README.md has been updated with the new tools, and a Showboat demo (demos/notifications-management.md) is provided.

Closes #85 ## The Need Currently, the `forgejo-mcp` server exposes the `check_notifications` tool, which is excellent for passively polling unread notifications. However, for fully autonomous agents that rely heavily on the MCP server to interact with Forgejo, merely reading notifications is insufficient. Without write access to the notification API, agents eventually get stuck or require manual GUI intervention to acknowledge notifications. ## The Solution This PR expands the `User/Notifications` toolset in `forgejo-mcp` to expose the full Forgejo SDK notification suite, allowing 100% headless notification management. ### Implemented Tools 1. `get_notification_thread` (Fetches a single thread by ID) 2. `mark_notification_read` (Marks a specific thread as read) 3. `mark_all_notifications_read` (Global acknowledgment) 4. `list_repo_notifications` (Filters notifications scoped to a specific repository) 5. `mark_repo_notifications_read` (Marks all notifications in a specific repository as read) ### Implementation Details - **Context/Logging:** Follows the established `log.LogMCPToolStart`/`LogMCPToolComplete` and `forgejo.LogAPICall` patterns. - **TDD:** The `mockNotificationServer` in `notification_test.go` has been expanded to test all HTTP endpoints and correctly handle empty JSON responses for `PUT` and `PATCH`. - **Documentation:** `README.md` has been updated with the new tools, and a `Showboat` demo (`demos/notifications-management.md`) is provided.

brenner-axiom commented

2026-03-08 10:02:11 +00:00

(Migrated from codeberg.org)

✅ What looks good

All 5 new tools follow the established logging/timing pattern from the rest of the codebase (log.WithMCPContext, LogMCPToolStart, LogMCPToolComplete, LogMCPToolError)
Correct nil-guard on resp before resp.StatusCode — this fixes the pre-existing bug documented in the race test suite
owner/repo required-param validation is explicit and returns a proper error, not just a silent zero-value
id validation in GetNotificationThreadFn and MarkNotificationReadFn returns a clear error message
All 6 tools (including the updated check_notifications) have test coverage
README table updated correctly

🔴 Must fix (requesting changes)

1. Demo uses a hardcoded fake token

demos/notifications-management.md bash blocks contain:

FORGEJO_ACCESS_TOKEN=dummy_token_12345

Even though this is not a real credential, the project recently went through a P0 token-scrub on PR #83. Consistent practice — and the project standard — is to reference $CODEBERG_TOKEN (or $FORGEJO_ACCESS_TOKEN) as a shell variable, never inline any string in that position.

Suggested fix for both exec blocks:

FORGEJO_URL=https://codeberg.org FORGEJO_ACCESS_TOKEN=$CODEBERG_TOKEN forgejo-mcp --cli ...

2. Silent time-parse failures in `since`/`before`

In CheckNotificationsFn and ListRepoNotificationsFn, invalid RFC3339 strings for since/before are silently swallowed:

if t, err := time.Parse(time.RFC3339, sinceStr); err == nil {
    opt.Since = t
}

The caller gets no feedback that their filter was ignored. The existing ListIssueCommentsFn (in operation/issue/issue.go) returns a proper error for the same case:

sinceTime, err := time.Parse(time.RFC3339, since)
if err != nil {
    return to.ErrorResult(fmt.Errorf("invalid since time format (expected RFC3339): %v", err))
}

Please apply the same pattern here.

🟡 Suggestions (non-blocking)

3. Race test `minimalArgs` not updated

test/race/race_test.go → minimalArgs() has no cases for the 5 new tools. The tools that need id (get_notification_thread, mark_notification_read) will silently use the default args map (which lacks id) — meaning the race test exercises the error path rather than the happy path for those tools.

Suggested addition:

case "get_notification_thread", "mark_notification_read":
    base["id"] = float64(1)
    delete(base, "owner")
    delete(base, "repo")
case "mark_all_notifications_read", "check_notifications":
    delete(base, "owner")
    delete(base, "repo")
case "list_repo_notifications", "mark_repo_notifications_read":
    // needs owner + repo (already set)

4. `MarkNotificationReadFn` discards the returned thread

_, resp, err := forgejo.Client().ReadNotification(int64(id))

The SDK returns the updated *Notification object. Returning it (as to.TextResult(thread)) would let the caller confirm what was marked read, consistent with how other mutating tools return the modified resource.

Overall this is a solid, well-tested PR that completes the notification surface area. The two 🔴 items are small fixes — happy to approve once they're addressed.

Great work @byteflavour — the implementation is clean, well-structured, and consistent with the existing codebase patterns. A few findings below, two of which I would ask you to address before merging. ## ✅ What looks good - All 5 new tools follow the established logging/timing pattern from the rest of the codebase (`log.WithMCPContext`, `LogMCPToolStart`, `LogMCPToolComplete`, `LogMCPToolError`) - **Correct nil-guard on `resp`** before `resp.StatusCode` — this fixes the pre-existing bug documented in the race test suite - `owner`/`repo` required-param validation is explicit and returns a proper error, not just a silent zero-value - `id` validation in `GetNotificationThreadFn` and `MarkNotificationReadFn` returns a clear error message - All 6 tools (including the updated `check_notifications`) have test coverage - README table updated correctly --- ## 🔴 Must fix (requesting changes) ### 1. Demo uses a hardcoded fake token `demos/notifications-management.md` bash blocks contain: ``` FORGEJO_ACCESS_TOKEN=dummy_token_12345 ``` Even though this is not a real credential, the project recently went through a P0 token-scrub on PR #83. Consistent practice — and the project standard — is to reference `$CODEBERG_TOKEN` (or `$FORGEJO_ACCESS_TOKEN`) as a shell variable, never inline any string in that position. Suggested fix for both exec blocks: ``` FORGEJO_URL=https://codeberg.org FORGEJO_ACCESS_TOKEN=$CODEBERG_TOKEN forgejo-mcp --cli ... ``` ### 2. Silent time-parse failures in `since`/`before` In `CheckNotificationsFn` and `ListRepoNotificationsFn`, invalid RFC3339 strings for `since`/`before` are silently swallowed: ```go if t, err := time.Parse(time.RFC3339, sinceStr); err == nil { opt.Since = t } ``` The caller gets no feedback that their filter was ignored. The existing `ListIssueCommentsFn` (in `operation/issue/issue.go`) returns a proper error for the same case: ```go sinceTime, err := time.Parse(time.RFC3339, since) if err != nil { return to.ErrorResult(fmt.Errorf("invalid since time format (expected RFC3339): %v", err)) } ``` Please apply the same pattern here. --- ## 🟡 Suggestions (non-blocking) ### 3. Race test `minimalArgs` not updated `test/race/race_test.go` → `minimalArgs()` has no cases for the 5 new tools. The tools that need `id` (`get_notification_thread`, `mark_notification_read`) will silently use the default args map (which lacks `id`) — meaning the race test exercises the error path rather than the happy path for those tools. Suggested addition: ```go case "get_notification_thread", "mark_notification_read": base["id"] = float64(1) delete(base, "owner") delete(base, "repo") case "mark_all_notifications_read", "check_notifications": delete(base, "owner") delete(base, "repo") case "list_repo_notifications", "mark_repo_notifications_read": // needs owner + repo (already set) ``` ### 4. `MarkNotificationReadFn` discards the returned thread ```go _, resp, err := forgejo.Client().ReadNotification(int64(id)) ``` The SDK returns the updated `*Notification` object. Returning it (as `to.TextResult(thread)`) would let the caller confirm what was marked read, consistent with how other mutating tools return the modified resource. --- Overall this is a solid, well-tested PR that completes the notification surface area. The two 🔴 items are small fixes — happy to approve once they're addressed.

goern commented

2026-03-08 10:21:36 +00:00

(Migrated from codeberg.org)

this is nice @byteflavour good contribution and good work, thanks!!

byteflavour commented

2026-03-08 10:29:40 +00:00

(Migrated from codeberg.org)

Is a favour for me :) The tool worx great and actually it's funny to contribute here and work with the mcp. Also the workflow and response-time is great. Welcome to the a.i. century :)

No reviewers

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

agentic-forges/forgejo-mcp!86

No description provided.

Rows
Columns

feat(notifications): implement complete notification API #86

The Need

The Solution

Implemented Tools

Implementation Details

✅ What looks good

🔴 Must fix (requesting changes)

1. Demo uses a hardcoded fake token

2. Silent time-parse failures in since/before

🟡 Suggestions (non-blocking)

3. Race test minimalArgs not updated

4. MarkNotificationReadFn discards the returned thread

2. Silent time-parse failures in `since`/`before`

3. Race test `minimalArgs` not updated

4. `MarkNotificationReadFn` discards the returned thread