diff --git a/README.md b/README.md index 145281bcdb..366c1d1b36 100644 --- a/README.md +++ b/README.md @@ -1601,6 +1601,7 @@ The behavior of lockdown mode depends on the tool invoked. Following tools will return an error when the author lacks the push access: - `issue_read:get` +- `issue_read:get_event` - `pull_request_read:get` - `pull_request_read:get_diff` - `pull_request_read:get_files` @@ -1610,6 +1611,8 @@ Following tools will filter out content from users lacking the push access: - `issue_read:get_comments` - `issue_read:get_sub_issues` +- `issue_read:get_events` +- `issue_read:get_timeline` - `pull_request_read:get_comments` - `pull_request_read:get_review_comments` - `pull_request_read:get_reviews` diff --git a/docs/feature-flags.md b/docs/feature-flags.md index 0ed3f9dc0e..f666a6ddf5 100644 --- a/docs/feature-flags.md +++ b/docs/feature-flags.md @@ -354,6 +354,28 @@ runtime behavior (such as output formatting) won't appear here. - `perPage`: Results per page for pagination (min 1, max 100) (number, optional) - `repo`: The name of the repository (string, required) +### `issue_events` + +- **issue_read** - Get issue details + - **OAuth Challenge Scopes**: `repo` + - `event_id`: The ID of the issue event. Required for, and only used by, the get_event method. (number, optional) + - `issue_number`: The number of the issue. Required for every method except get_event. (number, optional) + - `method`: The read operation to perform on a single issue. + Options are: + 1. get - Get issue details. Also returns best-effort hierarchy flags (`has_parent`, `has_children`); `parent` and `sub_issues_summary` are optional relationship summaries, and `closed_by_pull_requests` summarizes the pull requests configured to close the issue as `total_count` plus up to 5 `references`. + 2. get_comments - Get issue comments. + 3. get_sub_issues - Get sub-issues (children) of the issue. + 4. get_parent - Get the parent issue, if this issue is a sub-issue of another. + 5. get_labels - Get labels assigned to the issue. + 6. get_events - Get the issue's event history: labeled, assigned, closed, renamed, referenced and so on. + 7. get_timeline - Get the issue's timeline. A superset of get_events that also includes comments, commits and reviews, so prefer it when you need the full narrative and get_events when you only need state changes. + 8. get_event - Get a single issue event by its `event_id`. Takes `event_id` instead of `issue_number`. + (string, required) + - `owner`: The owner of the repository (string, required) + - `page`: Page number for pagination (min 1) (number, optional) + - `perPage`: Results per page for pagination (min 1, max 100) (number, optional) + - `repo`: The name of the repository (string, required) + ### `thread_resolution_reason` - **pull_request_review_write** - Write operations (create, submit, delete) on pull request reviews diff --git a/pkg/github/__toolsnaps__/issue_read_ff_issue_events.snap b/pkg/github/__toolsnaps__/issue_read_ff_issue_events.snap new file mode 100644 index 0000000000..d0d22b4723 --- /dev/null +++ b/pkg/github/__toolsnaps__/issue_read_ff_issue_events.snap @@ -0,0 +1,60 @@ +{ + "annotations": { + "idempotentHint": false, + "readOnlyHint": true, + "title": "Get issue details" + }, + "description": "Get information about a specific issue in a GitHub repository.", + "inputSchema": { + "properties": { + "event_id": { + "description": "The ID of the issue event. Required for, and only used by, the get_event method.", + "type": "number" + }, + "issue_number": { + "description": "The number of the issue. Required for every method except get_event.", + "type": "number" + }, + "method": { + "description": "The read operation to perform on a single issue.\nOptions are:\n1. get - Get issue details. Also returns best-effort hierarchy flags (`has_parent`, `has_children`); `parent` and `sub_issues_summary` are optional relationship summaries, and `closed_by_pull_requests` summarizes the pull requests configured to close the issue as `total_count` plus up to 5 `references`.\n2. get_comments - Get issue comments.\n3. get_sub_issues - Get sub-issues (children) of the issue.\n4. get_parent - Get the parent issue, if this issue is a sub-issue of another.\n5. get_labels - Get labels assigned to the issue.\n6. get_events - Get the issue's event history: labeled, assigned, closed, renamed, referenced and so on.\n7. get_timeline - Get the issue's timeline. A superset of get_events that also includes comments, commits and reviews, so prefer it when you need the full narrative and get_events when you only need state changes.\n8. get_event - Get a single issue event by its `event_id`. Takes `event_id` instead of `issue_number`.\n", + "enum": [ + "get", + "get_comments", + "get_sub_issues", + "get_parent", + "get_labels", + "get_events", + "get_timeline", + "get_event" + ], + "type": "string" + }, + "owner": { + "description": "The owner of the repository", + "type": "string" + }, + "page": { + "description": "Page number for pagination (min 1)", + "minimum": 1, + "type": "number" + }, + "perPage": { + "description": "Results per page for pagination (min 1, max 100)", + "maximum": 100, + "minimum": 1, + "type": "number" + }, + "repo": { + "description": "The name of the repository", + "type": "string" + } + }, + "required": [ + "method", + "owner", + "repo" + ], + "type": "object" + }, + "name": "issue_read" +} \ No newline at end of file diff --git a/pkg/github/feature_flags.go b/pkg/github/feature_flags.go index 27202c5c83..0fb0e80e99 100644 --- a/pkg/github/feature_flags.go +++ b/pkg/github/feature_flags.go @@ -34,6 +34,13 @@ const FeatureFlagIssueDependencies = "issue_dependencies" // opt-in. const FeatureFlagDuplicateDetection = "duplicate_detection" +// FeatureFlagIssueEvents is the feature flag name for the issue event history +// methods on issue_read (get_events, get_timeline, get_event), which expose an +// issue's event feed and timeline. It is gated so the extra methods and the +// event_id parameter are not advertised in the default issue_read schema, +// keeping the fixed tool-schema cost small unless explicitly opted in. +const FeatureFlagIssueEvents = "issue_events" + // FeatureFlagThreadResolutionReason exposes resolution reasons for Copilot review threads. const FeatureFlagThreadResolutionReason = "thread_resolution_reason" @@ -51,6 +58,7 @@ var AllowedFeatureFlags = []string{ FeatureFlagFileBlame, FeatureFlagIssueDependencies, FeatureFlagDuplicateDetection, + FeatureFlagIssueEvents, FeatureFlagThreadResolutionReason, } diff --git a/pkg/github/helper_test.go b/pkg/github/helper_test.go index 5fc541d45d..25d39c463f 100644 --- a/pkg/github/helper_test.go +++ b/pkg/github/helper_test.go @@ -62,6 +62,9 @@ const ( GetReposIssuesByOwnerByRepoByIssueNumber = "GET /repos/{owner}/{repo}/issues/{issue_number}" GetReposIssuesCommentByOwnerByRepoByCommentID = "GET /repos/{owner}/{repo}/issues/comments/{comment_id}" GetReposIssuesCommentsByOwnerByRepoByIssueNumber = "GET /repos/{owner}/{repo}/issues/{issue_number}/comments" + GetReposIssuesEventsByOwnerByRepoByIssueNumber = "GET /repos/{owner}/{repo}/issues/{issue_number}/events" + GetReposIssuesTimelineByOwnerByRepoByIssueNumber = "GET /repos/{owner}/{repo}/issues/{issue_number}/timeline" + GetReposIssuesEventByOwnerByRepoByEventID = "GET /repos/{owner}/{repo}/issues/events/{event_id}" PostReposIssuesByOwnerByRepo = "POST /repos/{owner}/{repo}/issues" PostReposIssuesCommentsByOwnerByRepoByIssueNumber = "POST /repos/{owner}/{repo}/issues/{issue_number}/comments" PostReposIssuesReactionsByOwnerByRepoByIssueNumber = "POST /repos/{owner}/{repo}/issues/{issue_number}/reactions" diff --git a/pkg/github/issues.go b/pkg/github/issues.go index 9b6ee5da6b..c47f704b69 100644 --- a/pkg/github/issues.go +++ b/pkg/github/issues.go @@ -7,6 +7,7 @@ import ( "fmt" "io" "net/http" + "slices" "strconv" "strings" "time" @@ -790,19 +791,53 @@ func isUnsupportedListIssuesIssueFieldsError(err error) bool { // IssueRead creates a tool to get details of a specific issue in a GitHub repository. func IssueRead(t translations.TranslationHelperFunc) inventory.ServerTool { + st := issueRead(t, false) + st.FeatureFlagDisable = []string{FeatureFlagIssueEvents} + return st +} + +// IssueReadWithEvents creates the feature-gated issue_read variant that also exposes the +// issue's event history: get_events, get_timeline and get_event. +func IssueReadWithEvents(t translations.TranslationHelperFunc) inventory.ServerTool { + st := issueRead(t, true) + st.FeatureFlagEnable = FeatureFlagIssueEvents + return st +} + +// issueRead builds the issue_read tool. When withEvents is false the schema and behavior are +// exactly the ungated tool. +func issueRead(t translations.TranslationHelperFunc, withEvents bool) inventory.ServerTool { + methodDescription := "The read operation to perform on a single issue.\n" + + "Options are:\n" + + "1. get - Get issue details. Also returns best-effort hierarchy flags (`has_parent`, `has_children`); `parent` and `sub_issues_summary` are optional relationship summaries, and `closed_by_pull_requests` summarizes the pull requests configured to close the issue as `total_count` plus up to 5 `references`.\n" + + "2. get_comments - Get issue comments.\n" + + "3. get_sub_issues - Get sub-issues (children) of the issue.\n" + + "4. get_parent - Get the parent issue, if this issue is a sub-issue of another.\n" + + "5. get_labels - Get labels assigned to the issue.\n" + methods := []any{"get", "get_comments", "get_sub_issues", "get_parent", "get_labels"} + + issueNumberDescription := "The number of the issue" + required := []string{"method", "owner", "repo", "issue_number"} + + if withEvents { + methodDescription += "6. get_events - Get the issue's event history: labeled, assigned, closed, renamed, referenced and so on.\n" + + "7. get_timeline - Get the issue's timeline. A superset of get_events that also includes comments, commits and reviews, so prefer it when you need the full narrative and get_events when you only need state changes.\n" + + "8. get_event - Get a single issue event by its `event_id`. Takes `event_id` instead of `issue_number`.\n" + methods = append(methods, "get_events", "get_timeline", "get_event") + + issueNumberDescription = "The number of the issue. Required for every method except get_event." + // get_event is addressed by event_id and has no issue number to supply, so + // issue_number drops out of the required list and is enforced per method below. + required = []string{"method", "owner", "repo"} + } + schema := &jsonschema.Schema{ Type: "object", Properties: map[string]*jsonschema.Schema{ "method": { - Type: "string", - Description: "The read operation to perform on a single issue.\n" + - "Options are:\n" + - "1. get - Get issue details. Also returns best-effort hierarchy flags (`has_parent`, `has_children`); `parent` and `sub_issues_summary` are optional relationship summaries, and `closed_by_pull_requests` summarizes the pull requests configured to close the issue as `total_count` plus up to 5 `references`.\n" + - "2. get_comments - Get issue comments.\n" + - "3. get_sub_issues - Get sub-issues (children) of the issue.\n" + - "4. get_parent - Get the parent issue, if this issue is a sub-issue of another.\n" + - "5. get_labels - Get labels assigned to the issue.\n", - Enum: []any{"get", "get_comments", "get_sub_issues", "get_parent", "get_labels"}, + Type: "string", + Description: methodDescription, + Enum: methods, }, "owner": { Type: "string", @@ -814,10 +849,16 @@ func IssueRead(t translations.TranslationHelperFunc) inventory.ServerTool { }, "issue_number": { Type: "number", - Description: "The number of the issue", + Description: issueNumberDescription, }, }, - Required: []string{"method", "owner", "repo", "issue_number"}, + Required: required, + } + if withEvents { + schema.Properties["event_id"] = &jsonschema.Schema{ + Type: "number", + Description: "The ID of the issue event. Required for, and only used by, the get_event method.", + } } WithPagination(schema) @@ -847,9 +888,19 @@ func IssueRead(t translations.TranslationHelperFunc) inventory.ServerTool { if err != nil { return utils.NewToolResultError(err.Error()), nil, nil } - issueNumber, err := RequiredInt(args, "issue_number") - if err != nil { - return utils.NewToolResultError(err.Error()), nil, nil + // The event history methods exist only on the feature-gated variant, so reject + // them here in case a client calls one without honoring the schema. + if !withEvents && slices.Contains([]string{"get_events", "get_timeline", "get_event"}, method) { + return utils.NewToolResultError(fmt.Sprintf("unknown method: %s", method)), nil, nil + } + + // get_event is addressed by event_id, so it is the one method with no issue number. + var issueNumber int + if method != "get_event" { + issueNumber, err = RequiredInt(args, "issue_number") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } } pagination, err := OptionalPaginationParams(args) @@ -888,6 +939,19 @@ func IssueRead(t translations.TranslationHelperFunc) inventory.ServerTool { case "get_labels": result, err := GetIssueLabels(ctx, gqlClient, owner, repo, issueNumber) return attachIFC(result), nil, err + case "get_events": + result, err := GetIssueEvents(ctx, client, deps, owner, repo, issueNumber, pagination) + return attachIFC(result), nil, err + case "get_timeline": + result, err := GetIssueTimeline(ctx, client, deps, owner, repo, issueNumber, pagination) + return attachIFC(result), nil, err + case "get_event": + eventID, err := RequiredBigInt(args, "event_id") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + result, err := GetIssueEvent(ctx, client, deps, owner, repo, eventID) + return attachIFC(result), nil, err default: return utils.NewToolResultError(fmt.Sprintf("unknown method: %s", method)), nil, nil } @@ -1062,6 +1126,179 @@ func GetIssueComments(ctx context.Context, client *github.Client, deps ToolDepen return MarshalledTextResult(minimalComments), nil } +// eventActorLogin reports the login to attribute an event or timeline entry to. `commented` +// and `reviewed` entries name their author in User and leave Actor empty, so both fields are +// consulted before an entry is treated as unattributable. +func eventActorLogin(actor, user *github.User) string { + if login := actor.GetLogin(); login != "" { + return login + } + return user.GetLogin() +} + +// GetIssueEvents returns the event history of an issue: the state changes (labeled, assigned, +// closed, renamed, ...) without the comment bodies that get_timeline adds. +func GetIssueEvents(ctx context.Context, client *github.Client, deps ToolDependencies, owner string, repo string, issueNumber int, pagination PaginationParams) (*mcp.CallToolResult, error) { + cache, err := deps.GetRepoAccessCache(ctx) + if err != nil { + return nil, fmt.Errorf("failed to get repo access cache: %w", err) + } + flags := deps.GetFlags(ctx) + + opts := &github.ListOptions{ + Page: pagination.Page, + PerPage: pagination.PerPage, + } + + events, resp, err := client.Issues.ListIssueEvents(ctx, owner, repo, issueNumber, opts) + if err != nil { + return nil, fmt.Errorf("failed to get issue events: %w", err) + } + defer func() { _ = resp.Body.Close() }() + + if resp.StatusCode != http.StatusOK { + body, err := io.ReadAll(resp.Body) + if err != nil { + return nil, fmt.Errorf("failed to read response body: %w", err) + } + return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, "failed to get issue events", resp, body), nil + } + + // Events carry user-authored content (renamed titles), so under lockdown they are + // filtered by actor exactly as issue comments are. + if flags.LockdownMode { + if cache == nil { + return nil, fmt.Errorf("lockdown cache is not configured") + } + filtered := make([]*github.IssueEvent, 0, len(events)) + for _, event := range events { + if event == nil { + continue + } + login := eventActorLogin(event.GetActor(), nil) + if login == "" { + continue + } + isSafeContent, err := cache.IsSafeContent(ctx, login, owner, repo) + if err != nil { + return utils.NewToolResultError(fmt.Sprintf("failed to check lockdown mode: %v", err)), nil + } + if isSafeContent { + filtered = append(filtered, event) + } + } + events = filtered + } + + minimalEvents := make([]MinimalIssueEvent, 0, len(events)) + for _, event := range events { + if event == nil { + continue + } + minimalEvents = append(minimalEvents, convertToMinimalIssueEvent(event)) + } + + return MarshalledTextResult(minimalEvents), nil +} + +// GetIssueTimeline returns the timeline of an issue: a superset of GetIssueEvents that also +// includes comments, commits and reviews. +func GetIssueTimeline(ctx context.Context, client *github.Client, deps ToolDependencies, owner string, repo string, issueNumber int, pagination PaginationParams) (*mcp.CallToolResult, error) { + cache, err := deps.GetRepoAccessCache(ctx) + if err != nil { + return nil, fmt.Errorf("failed to get repo access cache: %w", err) + } + flags := deps.GetFlags(ctx) + + opts := &github.ListOptions{ + Page: pagination.Page, + PerPage: pagination.PerPage, + } + + items, resp, err := client.Issues.ListIssueTimeline(ctx, owner, repo, issueNumber, opts) + if err != nil { + return nil, fmt.Errorf("failed to get issue timeline: %w", err) + } + defer func() { _ = resp.Body.Close() }() + + if resp.StatusCode != http.StatusOK { + body, err := io.ReadAll(resp.Body) + if err != nil { + return nil, fmt.Errorf("failed to read response body: %w", err) + } + return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, "failed to get issue timeline", resp, body), nil + } + + // The timeline embeds comment and review bodies, so an entry whose author lacks push + // access is dropped entirely under lockdown. + if flags.LockdownMode { + if cache == nil { + return nil, fmt.Errorf("lockdown cache is not configured") + } + filtered := make([]*github.Timeline, 0, len(items)) + for _, item := range items { + if item == nil { + continue + } + login := eventActorLogin(item.GetActor(), item.GetUser()) + if login == "" { + continue + } + isSafeContent, err := cache.IsSafeContent(ctx, login, owner, repo) + if err != nil { + return utils.NewToolResultError(fmt.Sprintf("failed to check lockdown mode: %v", err)), nil + } + if isSafeContent { + filtered = append(filtered, item) + } + } + items = filtered + } + + minimalItems := make([]MinimalTimelineItem, 0, len(items)) + for _, item := range items { + if item == nil { + continue + } + minimalItems = append(minimalItems, convertToMinimalTimelineItem(item)) + } + + return MarshalledTextResult(minimalItems), nil +} + +// GetIssueEvent returns a single issue event addressed by its own id. With only one event to +// return, lockdown mode rejects the read outright instead of filtering, as `get` does. +func GetIssueEvent(ctx context.Context, client *github.Client, deps ToolDependencies, owner string, repo string, eventID int64) (*mcp.CallToolResult, error) { + cache, err := deps.GetRepoAccessCache(ctx) + if err != nil { + return nil, fmt.Errorf("failed to get repo access cache: %w", err) + } + flags := deps.GetFlags(ctx) + + event, resp, err := client.Issues.GetEvent(ctx, owner, repo, eventID) + if err != nil { + return nil, fmt.Errorf("failed to get issue event: %w", err) + } + defer func() { _ = resp.Body.Close() }() + + if resp.StatusCode != http.StatusOK { + body, err := io.ReadAll(resp.Body) + if err != nil { + return nil, fmt.Errorf("failed to read response body: %w", err) + } + return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, "failed to get issue event", resp, body), nil + } + + if flags.LockdownMode { + login := eventActorLogin(event.GetActor(), nil) + if restricted, err := authorLockdownResult(ctx, cache, owner, repo, login, lockdownIssueEventRestrictedMessage); restricted != nil || err != nil { + return restricted, err + } + } + + return MarshalledTextResult(convertToMinimalIssueEvent(event)), nil +} + func GetSubIssues(ctx context.Context, client *github.Client, deps ToolDependencies, owner string, repo string, issueNumber int, pagination PaginationParams) (*mcp.CallToolResult, error) { cache, err := deps.GetRepoAccessCache(ctx) if err != nil { diff --git a/pkg/github/issues_events_test.go b/pkg/github/issues_events_test.go new file mode 100644 index 0000000000..37f3523a1a --- /dev/null +++ b/pkg/github/issues_events_test.go @@ -0,0 +1,787 @@ +package github + +import ( + "context" + "encoding/json" + "net/http" + "testing" + "time" + + "github.com/github/github-mcp-server/internal/toolsnaps" + "github.com/github/github-mcp-server/pkg/translations" + "github.com/google/go-github/v89/github" + "github.com/google/jsonschema-go/jsonschema" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// issueEventsDeps builds the dependencies for an issue_read call, optionally with lockdown mode +// enabled and backed by a permission server where "maintainer" has push access and every other +// login does not. +func issueEventsDeps(t *testing.T, mockedClient *http.Client, lockdownEnabled bool) BaseDeps { + t.Helper() + + var restClient *github.Client + if lockdownEnabled { + restClient = mockRESTPermissionServer(t, "read", map[string]string{ + "maintainer": "write", + "outsider": "read", + }) + } + + return BaseDeps{ + Client: mustNewGHClient(t, mockedClient), + GQLClient: defaultGQLClient, + RepoAccessCache: stubRepoAccessCache(restClient, 15*time.Minute), + Flags: stubFeatureFlags(map[string]bool{"lockdown-mode": lockdownEnabled}), + } +} + +func Test_IssueReadWithEvents_ToolDefinition(t *testing.T) { + t.Parallel() + + serverTool := IssueReadWithEvents(translations.NullTranslationHelper) + tool := serverTool.Tool + require.NoError(t, toolsnaps.Test(tool.Name+"_ff_"+FeatureFlagIssueEvents, tool)) + + assert.Equal(t, "issue_read", tool.Name) + assert.Equal(t, FeatureFlagIssueEvents, serverTool.FeatureFlagEnable) + assert.True(t, tool.Annotations.ReadOnlyHint) + + schema := tool.InputSchema.(*jsonschema.Schema) + assert.Contains(t, schema.Properties, "event_id") + assert.Subset(t, schema.Properties["method"].Enum, []any{"get_events", "get_timeline", "get_event"}) + + // issue_number leaves the required list on this variant so get_event, which is addressed by + // event_id, does not have to invent one. + assert.ElementsMatch(t, schema.Required, []string{"method", "owner", "repo"}) +} + +// Test_IssueRead_WithoutEventsFlag asserts the ungated tool is untouched by this feature: it +// neither advertises nor serves the event history methods. +func Test_IssueRead_WithoutEventsFlag(t *testing.T) { + t.Parallel() + + serverTool := IssueRead(translations.NullTranslationHelper) + schema := serverTool.Tool.InputSchema.(*jsonschema.Schema) + + assert.Equal(t, []string{FeatureFlagIssueEvents}, serverTool.FeatureFlagDisable) + assert.Empty(t, serverTool.FeatureFlagEnable) + assert.NotContains(t, schema.Properties, "event_id") + assert.NotContains(t, schema.Properties["method"].Enum, "get_events") + assert.NotContains(t, schema.Properties["method"].Enum, "get_timeline") + assert.NotContains(t, schema.Properties["method"].Enum, "get_event") + assert.ElementsMatch(t, schema.Required, []string{"method", "owner", "repo", "issue_number"}) + + for _, method := range []string{"get_events", "get_timeline", "get_event"} { + t.Run(method, func(t *testing.T) { + deps := issueEventsDeps(t, MockHTTPClientWithHandlers(map[string]http.HandlerFunc{}), false) + request := createMCPRequest(map[string]any{ + "method": method, + "owner": "owner", + "repo": "repo", + "issue_number": float64(42), + "event_id": float64(1), + }) + + result, err := serverTool.Handler(deps)(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + assert.Contains(t, getErrorResult(t, result).Text, "unknown method: "+method) + }) + } +} + +func Test_GetIssueEvents(t *testing.T) { + serverTool := IssueReadWithEvents(translations.NullTranslationHelper) + + mockEvents := []*github.IssueEvent{ + { + ID: github.Ptr(int64(123)), + Event: github.Ptr("labeled"), + Actor: &github.User{Login: github.Ptr("maintainer")}, + Label: &github.Label{Name: "bug"}, + CreatedAt: &github.Timestamp{Time: time.Date(2025, 5, 22, 10, 0, 0, 0, time.UTC)}, + }, + { + ID: github.Ptr(int64(456)), + Event: github.Ptr("renamed"), + Actor: &github.User{Login: github.Ptr("outsider")}, + Rename: &github.Rename{From: github.Ptr("old title"), To: github.Ptr("new title")}, + CommitID: github.Ptr("abc123"), + }, + } + + tests := []struct { + name string + mockedClient *http.Client + requestArgs map[string]any + lockdownEnabled bool + expectError bool + expectedErrMsg string + expectedEvents []MinimalIssueEvent + }{ + { + name: "successful events retrieval", + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + GetReposIssuesEventsByOwnerByRepoByIssueNumber: mockResponse(t, http.StatusOK, mockEvents), + }), + requestArgs: map[string]any{ + "method": "get_events", + "owner": "owner", + "repo": "repo", + "issue_number": float64(42), + }, + expectedEvents: []MinimalIssueEvent{ + { + ID: 123, + Event: "labeled", + Label: "bug", + CreatedAt: "2025-05-22T10:00:00Z", + }, + { + ID: 456, + Event: "renamed", + CommitID: "abc123", + RenamedFrom: "old title", + RenamedTo: "new title", + }, + }, + }, + { + name: "successful events retrieval with pagination", + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + GetReposIssuesEventsByOwnerByRepoByIssueNumber: expectQueryParams(t, map[string]string{ + "page": "2", + "per_page": "10", + }).andThen( + mockResponse(t, http.StatusOK, mockEvents), + ), + }), + requestArgs: map[string]any{ + "method": "get_events", + "owner": "owner", + "repo": "repo", + "issue_number": float64(42), + "page": float64(2), + "perPage": float64(10), + }, + expectedEvents: []MinimalIssueEvent{{ID: 123}, {ID: 456}}, + }, + { + name: "issue not found", + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + GetReposIssuesEventsByOwnerByRepoByIssueNumber: mockResponse(t, http.StatusNotFound, `{"message": "Not Found"}`), + }), + requestArgs: map[string]any{ + "method": "get_events", + "owner": "owner", + "repo": "repo", + "issue_number": float64(999), + }, + expectError: true, + expectedErrMsg: "failed to get issue events", + }, + { + name: "missing issue_number", + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + GetReposIssuesEventsByOwnerByRepoByIssueNumber: mockResponse(t, http.StatusOK, mockEvents), + }), + requestArgs: map[string]any{ + "method": "get_events", + "owner": "owner", + "repo": "repo", + }, + expectedErrMsg: "missing required parameter: issue_number", + }, + { + name: "lockdown filters events from actors without push access", + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + GetReposIssuesEventsByOwnerByRepoByIssueNumber: mockResponse(t, http.StatusOK, mockEvents), + }), + requestArgs: map[string]any{ + "method": "get_events", + "owner": "owner", + "repo": "repo", + "issue_number": float64(42), + }, + lockdownEnabled: true, + expectedEvents: []MinimalIssueEvent{ + {ID: 123, Event: "labeled", Label: "bug", CreatedAt: "2025-05-22T10:00:00Z"}, + }, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + deps := issueEventsDeps(t, tc.mockedClient, tc.lockdownEnabled) + request := createMCPRequest(tc.requestArgs) + + result, err := serverTool.Handler(deps)(ContextWithDeps(context.Background(), deps), &request) + + if tc.expectError { + require.Error(t, err) + assert.Contains(t, err.Error(), tc.expectedErrMsg) + return + } + require.NoError(t, err) + require.NotNil(t, result) + + if tc.expectedEvents == nil { + assert.Contains(t, getErrorResult(t, result).Text, tc.expectedErrMsg) + return + } + + var returned []MinimalIssueEvent + require.NoError(t, json.Unmarshal([]byte(getTextResult(t, result).Text), &returned)) + require.Len(t, returned, len(tc.expectedEvents)) + for i, expected := range tc.expectedEvents { + assert.Equal(t, expected.ID, returned[i].ID) + if expected.Event != "" { + assert.Equal(t, expected.Event, returned[i].Event) + assert.Equal(t, expected.Label, returned[i].Label) + assert.Equal(t, expected.CommitID, returned[i].CommitID) + assert.Equal(t, expected.RenamedFrom, returned[i].RenamedFrom) + assert.Equal(t, expected.RenamedTo, returned[i].RenamedTo) + assert.Equal(t, expected.CreatedAt, returned[i].CreatedAt) + } + } + }) + } +} + +func Test_GetIssueTimeline(t *testing.T) { + serverTool := IssueReadWithEvents(translations.NullTranslationHelper) + + mockTimeline := []*github.Timeline{ + { + ID: github.Ptr(int64(123)), + Event: github.Ptr("commented"), + User: &github.User{Login: github.Ptr("maintainer")}, + Body: github.Ptr("A comment on the issue"), + CreatedAt: &github.Timestamp{Time: time.Date(2025, 5, 22, 10, 0, 0, 0, time.UTC)}, + }, + { + ID: github.Ptr(int64(456)), + Event: github.Ptr("cross-referenced"), + Actor: &github.User{Login: github.Ptr("maintainer")}, + Source: &github.Source{ + Type: github.Ptr("issue"), + Actor: &github.User{Login: github.Ptr("maintainer")}, + Issue: &github.Issue{ + Number: github.Ptr(7), + Title: github.Ptr("Referring issue"), + State: github.Ptr("open"), + HTMLURL: github.Ptr("https://github.com/owner/repo/issues/7"), + }, + }, + }, + { + ID: github.Ptr(int64(789)), + Event: github.Ptr("committed"), + Actor: &github.User{Login: github.Ptr("outsider")}, + SHA: github.Ptr("def456"), + Message: github.Ptr("Fix the thing"), + }, + } + + tests := []struct { + name string + mockedClient *http.Client + requestArgs map[string]any + lockdownEnabled bool + expectError bool + expectedErrMsg string + expectedIDs []int64 + }{ + { + name: "successful timeline retrieval", + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + GetReposIssuesTimelineByOwnerByRepoByIssueNumber: mockResponse(t, http.StatusOK, mockTimeline), + }), + requestArgs: map[string]any{ + "method": "get_timeline", + "owner": "owner", + "repo": "repo", + "issue_number": float64(42), + }, + expectedIDs: []int64{123, 456, 789}, + }, + { + name: "successful timeline retrieval with pagination", + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + GetReposIssuesTimelineByOwnerByRepoByIssueNumber: expectQueryParams(t, map[string]string{ + "page": "3", + "per_page": "25", + }).andThen( + mockResponse(t, http.StatusOK, mockTimeline), + ), + }), + requestArgs: map[string]any{ + "method": "get_timeline", + "owner": "owner", + "repo": "repo", + "issue_number": float64(42), + "page": float64(3), + "perPage": float64(25), + }, + expectedIDs: []int64{123, 456, 789}, + }, + { + name: "issue not found", + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + GetReposIssuesTimelineByOwnerByRepoByIssueNumber: mockResponse(t, http.StatusNotFound, `{"message": "Not Found"}`), + }), + requestArgs: map[string]any{ + "method": "get_timeline", + "owner": "owner", + "repo": "repo", + "issue_number": float64(999), + }, + expectError: true, + expectedErrMsg: "failed to get issue timeline", + }, + { + // The `commented` entry is attributed via User rather than Actor, so this also + // covers the User fallback in eventActorLogin. + name: "lockdown filters timeline entries from users without push access", + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + GetReposIssuesTimelineByOwnerByRepoByIssueNumber: mockResponse(t, http.StatusOK, mockTimeline), + }), + requestArgs: map[string]any{ + "method": "get_timeline", + "owner": "owner", + "repo": "repo", + "issue_number": float64(42), + }, + lockdownEnabled: true, + expectedIDs: []int64{123, 456}, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + deps := issueEventsDeps(t, tc.mockedClient, tc.lockdownEnabled) + request := createMCPRequest(tc.requestArgs) + + result, err := serverTool.Handler(deps)(ContextWithDeps(context.Background(), deps), &request) + + if tc.expectError { + require.Error(t, err) + assert.Contains(t, err.Error(), tc.expectedErrMsg) + return + } + require.NoError(t, err) + + var returned []MinimalTimelineItem + require.NoError(t, json.Unmarshal([]byte(getTextResult(t, result).Text), &returned)) + + ids := make([]int64, 0, len(returned)) + for _, item := range returned { + ids = append(ids, item.ID) + } + assert.Equal(t, tc.expectedIDs, ids) + }) + } +} + +// Test_GetIssueTimeline_TrimsAndSanitizes pins the minimal shape: the comment body and the +// cross-referenced issue title survive, while the verbose upstream nesting does not. +func Test_GetIssueTimeline_TrimsAndSanitizes(t *testing.T) { + t.Parallel() + + serverTool := IssueReadWithEvents(translations.NullTranslationHelper) + deps := issueEventsDeps(t, MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + GetReposIssuesTimelineByOwnerByRepoByIssueNumber: mockResponse(t, http.StatusOK, []*github.Timeline{ + { + ID: github.Ptr(int64(1)), + Event: github.Ptr("cross-referenced"), + Actor: &github.User{Login: github.Ptr("maintainer")}, + Source: &github.Source{ + Type: github.Ptr("issue"), + Issue: &github.Issue{ + Number: github.Ptr(7), + Title: github.Ptr("Referring issue"), + State: github.Ptr("open"), + HTMLURL: github.Ptr("https://github.com/other/repo/issues/7"), + Repository: &github.Repository{FullName: github.Ptr("other/repo")}, + }, + }, + }, + }), + }), false) + + request := createMCPRequest(map[string]any{ + "method": "get_timeline", + "owner": "owner", + "repo": "repo", + "issue_number": float64(42), + }) + + result, err := serverTool.Handler(deps)(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + + var returned []MinimalTimelineItem + require.NoError(t, json.Unmarshal([]byte(getTextResult(t, result).Text), &returned)) + require.Len(t, returned, 1) + + require.NotNil(t, returned[0].Source) + require.NotNil(t, returned[0].Source.Issue) + assert.Equal(t, "issue", returned[0].Source.Type) + assert.Equal(t, 7, returned[0].Source.Issue.Number) + assert.Equal(t, "Referring issue", returned[0].Source.Issue.Title) + assert.Equal(t, "other/repo", returned[0].Source.Issue.Repository) + + // The trimmed entry must not carry the upstream commit-ancestry payload. + var raw []map[string]json.RawMessage + require.NoError(t, json.Unmarshal([]byte(getTextResult(t, result).Text), &raw)) + assert.NotContains(t, raw[0], "parents") +} + +func Test_GetIssueEvent(t *testing.T) { + serverTool := IssueReadWithEvents(translations.NullTranslationHelper) + + // The single-event endpoint is the one that returns the issue the event belongs to, since + // the caller addresses the event by id and never supplies an issue number. + mockEvent := &github.IssueEvent{ + ID: github.Ptr(int64(17196710688)), + Event: github.Ptr("closed"), + Actor: &github.User{Login: github.Ptr("maintainer")}, + CommitID: github.Ptr("abc123"), + CreatedAt: &github.Timestamp{Time: time.Date(2025, 5, 22, 10, 0, 0, 0, time.UTC)}, + // No nested Repository: the REST payload carries repository_url instead, so the ref's + // Repository stays empty and the issue is understood to be in the addressed repo. + Issue: &github.Issue{ + Number: github.Ptr(42), + Title: github.Ptr("Something broke"), + State: github.Ptr("closed"), + HTMLURL: github.Ptr("https://github.com/owner/repo/issues/42"), + }, + } + + tests := []struct { + name string + mockedClient *http.Client + requestArgs map[string]any + lockdownEnabled bool + expectError bool + expectResultErr bool + expectedErrMsg string + expectedEventID int64 + expectedActor string + expectedCommitID string + expectedIssueNum int + }{ + { + name: "successful single event retrieval", + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + GetReposIssuesEventByOwnerByRepoByEventID: mockResponse(t, http.StatusOK, mockEvent), + }), + requestArgs: map[string]any{ + "method": "get_event", + "owner": "owner", + "repo": "repo", + "event_id": float64(17196710688), + }, + expectedEventID: 17196710688, + expectedActor: "maintainer", + expectedCommitID: "abc123", + expectedIssueNum: 42, + }, + { + // get_event takes no issue_number, which is the reason it is left out of the + // schema-level required list. + name: "succeeds without issue_number", + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + GetReposIssuesEventByOwnerByRepoByEventID: mockResponse(t, http.StatusOK, mockEvent), + }), + requestArgs: map[string]any{ + "method": "get_event", + "owner": "owner", + "repo": "repo", + "event_id": float64(17196710688), + }, + expectedEventID: 17196710688, + expectedActor: "maintainer", + expectedCommitID: "abc123", + expectedIssueNum: 42, + }, + { + name: "missing event_id", + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + GetReposIssuesEventByOwnerByRepoByEventID: mockResponse(t, http.StatusOK, mockEvent), + }), + requestArgs: map[string]any{ + "method": "get_event", + "owner": "owner", + "repo": "repo", + }, + expectResultErr: true, + expectedErrMsg: "missing required parameter: event_id", + }, + { + name: "event not found", + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + GetReposIssuesEventByOwnerByRepoByEventID: mockResponse(t, http.StatusNotFound, `{"message": "Not Found"}`), + }), + requestArgs: map[string]any{ + "method": "get_event", + "owner": "owner", + "repo": "repo", + "event_id": float64(999), + }, + expectError: true, + expectedErrMsg: "failed to get issue event", + }, + { + name: "lockdown allows event from actor with push access", + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + GetReposIssuesEventByOwnerByRepoByEventID: mockResponse(t, http.StatusOK, mockEvent), + }), + requestArgs: map[string]any{ + "method": "get_event", + "owner": "owner", + "repo": "repo", + "event_id": float64(17196710688), + }, + lockdownEnabled: true, + expectedEventID: 17196710688, + expectedActor: "maintainer", + expectedCommitID: "abc123", + expectedIssueNum: 42, + }, + { + // A single event cannot be filtered down, so lockdown refuses the read outright. + name: "lockdown rejects event from actor without push access", + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + GetReposIssuesEventByOwnerByRepoByEventID: mockResponse(t, http.StatusOK, &github.IssueEvent{ + ID: github.Ptr(int64(555)), + Event: github.Ptr("renamed"), + Actor: &github.User{Login: github.Ptr("outsider")}, + }), + }), + requestArgs: map[string]any{ + "method": "get_event", + "owner": "owner", + "repo": "repo", + "event_id": float64(555), + }, + lockdownEnabled: true, + expectResultErr: true, + expectedErrMsg: "access to issue event is restricted by lockdown mode", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + deps := issueEventsDeps(t, tc.mockedClient, tc.lockdownEnabled) + request := createMCPRequest(tc.requestArgs) + + result, err := serverTool.Handler(deps)(ContextWithDeps(context.Background(), deps), &request) + + if tc.expectError { + require.Error(t, err) + assert.Contains(t, err.Error(), tc.expectedErrMsg) + return + } + require.NoError(t, err) + require.NotNil(t, result) + + if tc.expectResultErr { + assert.Contains(t, getErrorResult(t, result).Text, tc.expectedErrMsg) + return + } + + var returned MinimalIssueEvent + require.NoError(t, json.Unmarshal([]byte(getTextResult(t, result).Text), &returned)) + assert.Equal(t, tc.expectedEventID, returned.ID) + require.NotNil(t, returned.Actor) + assert.Equal(t, tc.expectedActor, returned.Actor.Login) + assert.Equal(t, tc.expectedCommitID, returned.CommitID) + if tc.expectedIssueNum == 0 { + assert.Nil(t, returned.Issue) + } else { + require.NotNil(t, returned.Issue) + assert.Equal(t, tc.expectedIssueNum, returned.Issue.Number) + assert.Equal(t, "Something broke", returned.Issue.Title) + assert.Empty(t, returned.Issue.Repository) + } + }) + } +} + +func Test_repoFullNameFromCommitURL(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + url string + expected string + }{ + { + name: "cross-repo commit on a referenced event", + url: "https://api.github.com/repos/saraycp/open-build-service/commits/5291b58b04", + expected: "saraycp/open-build-service", + }, + { + name: "fork branch on a force-push event", + url: "https://api.github.com/repos/artemsaveliev/github-mcp-server/commits/79b2a855ab", + expected: "artemsaveliev/github-mcp-server", + }, + { + name: "GHES host", + url: "https://github.example.com/api/v3/repos/owner/repo/commits/abc123", + expected: "owner/repo", + }, + {name: "empty url", url: "", expected: ""}, + {name: "no repos segment", url: "https://api.github.com/user", expected: ""}, + {name: "owner only", url: "https://api.github.com/repos/owner", expected: ""}, + {name: "trailing slash after owner", url: "https://api.github.com/repos/owner/", expected: ""}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + assert.Equal(t, tc.expected, repoFullNameFromCommitURL(tc.url)) + }) + } +} + +func Test_GetIssueTimeline_CarriesCommitRepository(t *testing.T) { + t.Parallel() + + // A bare commit_id is unresolvable when the commit lives outside this repo, which is the + // common case for referenced and force-push entries. + serverTool := IssueReadWithEvents(translations.NullTranslationHelper) + deps := issueEventsDeps(t, MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + GetReposIssuesTimelineByOwnerByRepoByIssueNumber: mockResponse(t, http.StatusOK, []*github.Timeline{ + { + Event: github.Ptr("referenced"), + Actor: &github.User{Login: github.Ptr("outsider")}, + CommitID: github.Ptr("5291b58b04"), + CommitURL: github.Ptr("https://api.github.com/repos/other/unrelated/commits/5291b58b04"), + }, + { + Event: github.Ptr("labeled"), + Actor: &github.User{Login: github.Ptr("maintainer")}, + Label: &github.Label{Name: "bug"}, + CommitID: github.Ptr(""), + }, + }), + }), false) + + request := createMCPRequest(map[string]any{ + "method": "get_timeline", + "owner": "owner", + "repo": "repo", + "issue_number": float64(42), + }) + + result, err := serverTool.Handler(deps)(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + + var returned []MinimalTimelineItem + require.NoError(t, json.Unmarshal([]byte(getTextResult(t, result).Text), &returned)) + require.Len(t, returned, 2) + + assert.Equal(t, "5291b58b04", returned[0].CommitID) + assert.Equal(t, "other/unrelated", returned[0].CommitRepository) + + // Entries with no commit stay clean rather than carrying an empty key. + assert.Empty(t, returned[1].CommitRepository) + var raw []map[string]json.RawMessage + require.NoError(t, json.Unmarshal([]byte(getTextResult(t, result).Text), &raw)) + assert.NotContains(t, raw[1], "commit_repository") +} + +func Test_GetIssueEvent_CarriesDismissedReview(t *testing.T) { + t.Parallel() + + serverTool := IssueReadWithEvents(translations.NullTranslationHelper) + deps := issueEventsDeps(t, MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + GetReposIssuesEventByOwnerByRepoByEventID: mockResponse(t, http.StatusOK, &github.IssueEvent{ + ID: github.Ptr(int64(99)), + Event: github.Ptr("review_dismissed"), + Actor: &github.User{Login: github.Ptr("maintainer")}, + DismissedReview: &github.DismissedReview{ + State: github.Ptr("changes_requested"), + ReviewID: github.Ptr(int64(456)), + DismissalMessage: github.Ptr("Stale after rebase"), + DismissalCommitID: github.Ptr("def456"), + }, + }), + }), false) + + request := createMCPRequest(map[string]any{ + "method": "get_event", + "owner": "owner", + "repo": "repo", + "event_id": float64(99), + }) + + result, err := serverTool.Handler(deps)(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + + var returned MinimalIssueEvent + require.NoError(t, json.Unmarshal([]byte(getTextResult(t, result).Text), &returned)) + + require.NotNil(t, returned.DismissedReview) + assert.Equal(t, "changes_requested", returned.DismissedReview.State) + assert.Equal(t, int64(456), returned.DismissedReview.ReviewID) + assert.Equal(t, "def456", returned.DismissedReview.DismissalCommitID) + // The dismissal message is author-supplied, so it is sanitized like any other body text. + assert.NotContains(t, returned.DismissedReview.DismissalMessage, "