From 3d47116b0ede800dd3e7ccf2887eb5c6ca84ad00 Mon Sep 17 00:00:00 2001 From: ysyneu Date: Wed, 8 Jul 2026 03:50:31 -0700 Subject: [PATCH] feat(pagination): expose page param on list-query tools MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The list-query MCP tools hit paginated backends (ListOptions{Page,Limit}) but exposed only `limit` (max 100) — any result past the first page was unreachable, so a query matching >100 items silently dropped the tail with no recourse but narrowing the window/filters. Add a 1-based `page` param to every list tool that queries a Page-capable endpoint and surfaces a `total`: query_incidents, query_changes, query_incident_alerts, query_channels, query_members, query_teams. The three that had no `limit` (channels/members/teams) gain both. Dedicated by-ID lookups that return everything (incident_ids→list-by-ids, person_ids→person/infos, team_ids→team/infos) stay unpaged; channel_ids is a filter on the same paginated /channel/list, so it pages like any listing. Fix the truncation signal for paged reads: `addTruncationHint` compared `total > count` (this page's size), which was correct only on page 1 — on a final partial page it falsely reported "truncated" and sent the agent chasing an empty next page. New `addPageHint` accounts for the page offset (seen = (page-1)*limit + count) and names the exact next page. Full-set / by-ID responses keep `addTruncationHint`. Shared `optionalPaging` helper centralizes the limit/page parse across the six handlers; `PageDescription` mirrors `LimitDescription`. Wire-level tests cover page reaching the backend as `p` (and default omitting it), the channel_ids/person_ids path split, and the last-page truncation math. Out of scope: list_similar_incidents (backend /incident/past-list has no Page — top-N by design). --- pkg/flashduty/changes.go | 28 ++- pkg/flashduty/channels.go | 14 +- pkg/flashduty/incidents.go | 37 ++-- pkg/flashduty/pagination.go | 66 +++++- pkg/flashduty/pagination_page_test.go | 308 ++++++++++++++++++++++++++ pkg/flashduty/users.go | 28 ++- 6 files changed, 440 insertions(+), 41 deletions(-) create mode 100644 pkg/flashduty/pagination_page_test.go diff --git a/pkg/flashduty/changes.go b/pkg/flashduty/changes.go index e025abf..bfbad48 100644 --- a/pkg/flashduty/changes.go +++ b/pkg/flashduty/changes.go @@ -29,6 +29,7 @@ func QueryChanges(getClient GetFlashdutyClientFn, t translations.TranslationHelp WithUntil(), mcp.WithString("type", mcp.Description("Filter by change type.")), mcp.WithNumber("limit", mcp.Description(LimitDescription), mcp.DefaultNumber(20), mcp.Min(1), mcp.Max(100)), + mcp.WithNumber("page", mcp.Description(PageDescription), mcp.DefaultNumber(1), mcp.Min(1)), ), func(ctx context.Context, request mcp.CallToolRequest) (*mcp.CallToolResult, error) { ctx, client, err := getClient(ctx) if err != nil { @@ -39,11 +40,7 @@ func QueryChanges(getClient GetFlashdutyClientFn, t translations.TranslationHelp changeIdsStr, _ := OptionalParam[string](request, "change_ids") channelIdsStr, _ := OptionalParam[string](request, "channel_ids") changeType, _ := OptionalParam[string](request, "type") - limit, _ := OptionalInt(request, "limit") - - if limit <= 0 { - limit = 20 - } + limit, page := optionalPaging(request, defaultQueryLimit) startTime, err := timeutil.ParseAny(args["since"]) if err != nil { @@ -71,6 +68,9 @@ func QueryChanges(getClient GetFlashdutyClientFn, t translations.TranslationHelp Query: changeType, } req.Limit = limit + if page > 1 { + req.Page = page + } if channelIdsStr != "" { channelIDs := parseCommaSeparatedInts(channelIdsStr) @@ -89,8 +89,13 @@ func QueryChanges(getClient GetFlashdutyClientFn, t translations.TranslationHelp } changes := resp.Items - // /change/list has no change_ids filter; honor the direct-lookup - // param by filtering the returned page client-side. + // /change/list has no server-side change_ids filter; honor the + // direct-lookup param by filtering the fetched page client-side. + // This is a best-effort single-page lookup — paging doesn't map onto + // it, so report the found count as the total and skip the page hint. + // (Feeding the filtered count with the window's unfiltered total into + // addPageHint would mislead: "Showing 0 of 5000, request page:2", + // inviting the agent to page the whole window chasing a few IDs.) if changeIdsStr != "" { wanted := make(map[string]struct{}) for _, id := range parseCommaSeparatedStrings(changeIdsStr) { @@ -102,12 +107,15 @@ func QueryChanges(getClient GetFlashdutyClientFn, t translations.TranslationHelp filtered = append(filtered, ch) } } - changes = filtered + return MarshalResult(map[string]any{ + "changes": filtered, + "total": len(filtered), + }), nil } - return MarshalResult(addTruncationHint(map[string]any{ + return MarshalResult(addPageHint(map[string]any{ "changes": changes, "total": resp.Total, - }, len(changes), int(resp.Total))), nil + }, len(changes), int(resp.Total), page, limit)), nil } } diff --git a/pkg/flashduty/channels.go b/pkg/flashduty/channels.go index 4a87b0a..915a509 100644 --- a/pkg/flashduty/channels.go +++ b/pkg/flashduty/channels.go @@ -23,6 +23,8 @@ func QueryChannels(getClient GetFlashdutyClientFn, t translations.TranslationHel }), mcp.WithString("channel_ids", mcp.Description("Comma-separated channel IDs for direct lookup. Max 1000 IDs.")), mcp.WithString("name", mcp.Description("Search by channel name (case-insensitive substring match).")), + mcp.WithNumber("limit", mcp.Description(LimitDescription), mcp.DefaultNumber(20), mcp.Min(1), mcp.Max(100)), + mcp.WithNumber("page", mcp.Description(PageDescription), mcp.DefaultNumber(1), mcp.Min(1)), ), func(ctx context.Context, request mcp.CallToolRequest) (*mcp.CallToolResult, error) { ctx, client, err := getClient(ctx) if err != nil { @@ -31,12 +33,20 @@ func QueryChannels(getClient GetFlashdutyClientFn, t translations.TranslationHel channelIdsStr, _ := OptionalParam[string](request, "channel_ids") name, _ := OptionalParam[string](request, "name") + limit, page := optionalPaging(request, defaultQueryLimit) // Map name to the free-text Query (substring match against // name/description) rather than ChannelName, which is exact-match. + // channel_ids is a filter on the same paginated /channel/list + // endpoint (not a dedicated by-ID lookup), so limit/page apply to it + // too — a >limit channel_ids lookup pages like any other listing. req := &flashduty.ListChannelsRequest{ Query: name, } + req.Limit = limit + if page > 1 { + req.Page = page + } // Parse channel IDs if provided if channelIdsStr != "" { @@ -58,10 +68,10 @@ func QueryChannels(getClient GetFlashdutyClientFn, t translations.TranslationHel } total := int(out.Total) - return MarshalResult(addTruncationHint(map[string]any{ + return MarshalResult(addPageHint(map[string]any{ "channels": out.Items, "total": total, - }, len(out.Items), total)), nil + }, len(out.Items), total, page, limit)), nil } } diff --git a/pkg/flashduty/incidents.go b/pkg/flashduty/incidents.go index b082e00..e3fdfed 100644 --- a/pkg/flashduty/incidents.go +++ b/pkg/flashduty/incidents.go @@ -44,6 +44,7 @@ func QueryIncidents(getClient GetFlashdutyClientFn, t translations.TranslationHe mcp.WithString("query", mcp.Description("Free-text search across title, labels, and content (Doris full-text). A 24-char hex string is resolved as an incident ID; a 6-char string is resolved as an incident num. Prefer this over picking exact filter values when the user gives a fuzzy keyword."), mcp.MaxLength(200)), mcp.WithString("nums", mcp.Description("Comma-separated short incident ids (num — the 6-char id shown in the UI, e.g. 311510). Matched within the since/until window; the backend caps the list span at ~30 days, so incidents older than that must be looked up by their full incident_id.")), mcp.WithNumber("limit", mcp.Description(LimitDescription), mcp.DefaultNumber(20), mcp.Min(1), mcp.Max(100)), + mcp.WithNumber("page", mcp.Description(PageDescription), mcp.DefaultNumber(1), mcp.Min(1)), ), func(ctx context.Context, request mcp.CallToolRequest) (*mcp.CallToolResult, error) { ctx, client, err := getClient(ctx) if err != nil { @@ -57,7 +58,7 @@ func QueryIncidents(getClient GetFlashdutyClientFn, t translations.TranslationHe channelIdsStr, _ := OptionalParam[string](request, "channel_ids") query, _ := OptionalParam[string](request, "query") nums, _ := OptionalParam[string](request, "nums") - limit, _ := OptionalInt(request, "limit") + limit, page := optionalPaging(request, defaultQueryLimit) startTime, err := timeutil.ParseAny(args["since"]) if err != nil { @@ -85,10 +86,6 @@ func QueryIncidents(getClient GetFlashdutyClientFn, t translations.TranslationHe startTime = endTime - int64(DefaultIncidentWindow/time.Second) } - if limit <= 0 { - limit = defaultQueryLimit - } - // Direct ID lookup uses /incident/list-by-ids (ListByIDs), which does // not require a time window. Per the tool contract, when incident_ids // is provided every other filter is ignored. @@ -125,6 +122,11 @@ func QueryIncidents(getClient GetFlashdutyClientFn, t translations.TranslationHe Query: query, } req.Limit = limit + // page 1 is the default; only send `p` when advancing so the + // wire payload stays unchanged for callers that don't paginate. + if page > 1 { + req.Page = page + } if channelIdsStr != "" { channelIDs := parseCommaSeparatedInts(channelIdsStr) @@ -151,10 +153,10 @@ func QueryIncidents(getClient GetFlashdutyClientFn, t translations.TranslationHe } total := int(out.Total) - return MarshalResult(addTruncationHint(map[string]any{ + return MarshalResult(addPageHint(map[string]any{ "incidents": out.Items, "total": total, - }, len(out.Items), total)), nil + }, len(out.Items), total, page, limit)), nil } } @@ -220,7 +222,8 @@ func QueryIncidentAlerts(getClient GetFlashdutyClientFn, t translations.Translat ReadOnlyHint: ToBoolPtr(true), }), mcp.WithString("incident_ids", mcp.Required(), mcp.Description("Comma-separated incident IDs to query alerts for.")), - mcp.WithNumber("limit", mcp.Description("Maximum alerts per incident."), mcp.DefaultNumber(20), mcp.Min(1), mcp.Max(100)), + mcp.WithNumber("limit", mcp.Description("Maximum alerts per incident per page. Default 20, max 100. When an incident has more alerts than returned, its entry carries `truncated:true` and a `hint`."), mcp.DefaultNumber(20), mcp.Min(1), mcp.Max(100)), + mcp.WithNumber("page", mcp.Description("1-based page number, applied to every requested incident. Default 1. When an incident's entry is `truncated`, request `page:2` (then 3, …) to fetch its remaining alerts."), mcp.DefaultNumber(1), mcp.Min(1)), ), func(ctx context.Context, request mcp.CallToolRequest) (*mcp.CallToolResult, error) { ctx, client, err := getClient(ctx) if err != nil { @@ -237,26 +240,28 @@ func QueryIncidentAlerts(getClient GetFlashdutyClientFn, t translations.Translat return mcp.NewToolResultError("incident_ids must contain at least one valid ID"), nil } - limit, _ := OptionalInt(request, "limit") - if limit <= 0 { - limit = defaultQueryLimit - } + limit, page := optionalPaging(request, defaultQueryLimit) // go-flashduty's Incidents.AlertList returns one incident's alerts - // per call, so fan out across the requested IDs. + // per call, so fan out across the requested IDs. The page applies + // uniformly to each incident's alert list. response := make([]map[string]any, 0, len(incidentIDs)) for _, id := range incidentIDs { alertReq := &flashduty.ListIncidentAlertsRequest{IncidentID: id} alertReq.Limit = limit + if page > 1 { + alertReq.Page = page + } out, _, err := client.New.Incidents.AlertList(ctx, alertReq) if err != nil { return mcp.NewToolResultError(fmt.Sprintf("Unable to retrieve alerts for %s: %v", id, err)), nil } - response = append(response, map[string]any{ + total := int(out.Total) + response = append(response, addPageHint(map[string]any{ "incident_id": id, "alerts": out.Items, - "total": int(out.Total), - }) + "total": total, + }, len(out.Items), total, page, limit)) } return MarshalResult(map[string]any{ diff --git a/pkg/flashduty/pagination.go b/pkg/flashduty/pagination.go index 1d8e2a4..549c7de 100644 --- a/pkg/flashduty/pagination.go +++ b/pkg/flashduty/pagination.go @@ -1,20 +1,45 @@ package flashduty -import "fmt" +import ( + "fmt" + + "github.com/mark3labs/mcp-go/mcp" +) // LimitDescription is the canonical description for the `limit` parameter on // list-shaped tools. Mentioning `truncated`/`total` up front primes the LLM to // inspect them before assuming it has the full result. -const LimitDescription = "Maximum number of results to return. Default 20, max 100. " + +const LimitDescription = "Maximum number of results per page. Default 20, max 100. " + "When more results exist than were returned, the response carries " + "`truncated:true` and a `hint` field with concrete next steps." +// PageDescription is the canonical description for the `page` parameter on +// list-shaped tools. Pairs with `limit`: page N returns results +// [(N-1)*limit, N*limit). Advertised alongside the `truncated`/`hint` fields so +// the LLM knows how to reach results beyond the first page. +const PageDescription = "1-based page number for paging through results beyond `limit`. " + + "Default 1. When the response is `truncated`, request `page:2` (then 3, …) to fetch the rest." + +// optionalPaging reads the shared `limit` and `page` parameters that every +// paginated list tool exposes, applying defLimit when limit is absent/invalid. +// Centralizing this keeps the six list handlers' paging contract identical. +func optionalPaging(r mcp.CallToolRequest, defLimit int) (limit, page int) { + limit, _ = OptionalInt(r, "limit") + if limit <= 0 { + limit = defLimit + } + page, _ = OptionalInt(r, "page") + return limit, page +} + // addTruncationHint stamps `truncated: true` and a human-readable `hint` onto // list-shaped results when the returned slice is shorter than the backend's -// total. Without these explicit fields the LLM has to remember to compare -// `len(items) < total`, and skipping that check is the most common cause of -// "the LLM only looked at the first 20 incidents and missed the obvious one" -// reports. +// total. Use it for full-set / by-ID responses that return everything matching +// in one call (no page parameter). For paginated tools use addPageHint, which +// accounts for the page offset. Without these explicit fields the LLM has to +// remember to compare `len(items) < total`, and skipping that check is the most +// common cause of "the LLM only looked at the first 20 incidents and missed the +// obvious one" reports. // // No-op when nothing was truncated, so happy-path output stays clean. // Returns the same map for one-line use. @@ -22,9 +47,36 @@ func addTruncationHint(res map[string]any, count, total int) map[string]any { if total > count { res["truncated"] = true res["hint"] = fmt.Sprintf( - "Returned %d of %d total. To see more: raise `limit` (max 100), narrow `since`/`until`, or add filters (severity, channel_ids, etc.).", + "Returned %d of %d total. To see more, raise `limit` (max 100) or narrow filters (time window, severity, channel_ids, etc.).", count, total, ) } return res } + +// addPageHint is the paginated counterpart to addTruncationHint. It flags +// `truncated` and names the next page only when the caller has NOT yet paged +// through everything: `count` is the current page's item count and page/limit +// describe the page just fetched (page is 1-based). This distinction matters — +// comparing `total > count` alone would spuriously flag the final partial page +// as truncated and send the agent chasing an empty next page. +// +// No-op on the last page (and on non-paginated callers where seen >= total), so +// happy-path output stays clean. Returns the same map for one-line use. +func addPageHint(res map[string]any, count, total, page, limit int) map[string]any { + if page < 1 { + page = 1 + } + if limit < 1 { + limit = count + } + seen := (page-1)*limit + count + if total > seen { + res["truncated"] = true + res["hint"] = fmt.Sprintf( + "Showing %d of %d so far (through page %d). Request `page:%d` for the next page, or raise `limit` (max 100) for bigger pages. Narrowing filters also shrinks the result set.", + seen, total, page, page+1, + ) + } + return res +} diff --git a/pkg/flashduty/pagination_page_test.go b/pkg/flashduty/pagination_page_test.go new file mode 100644 index 0000000..f51f892 --- /dev/null +++ b/pkg/flashduty/pagination_page_test.go @@ -0,0 +1,308 @@ +package flashduty + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "strings" + "testing" + + flashduty "github.com/flashcatcloud/go-flashduty" + "github.com/mark3labs/mcp-go/mcp" + + "github.com/flashcatcloud/flashduty-mcp-server/pkg/translations" +) + +// captureWire spins up a test backend that records the request path and decoded +// JSON body, then returns an empty list response. It lets a tool handler run +// end-to-end so we can assert what actually reached the wire. +func captureWire(t *testing.T) (*httptest.Server, *string, *map[string]any) { + t.Helper() + var gotPath string + gotBody := map[string]any{} + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + gotPath = r.URL.Path + _ = json.NewDecoder(r.Body).Decode(&gotBody) + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(map[string]any{ + "data": map[string]any{"items": []any{}, "total": 0}, + }) + })) + return ts, &gotPath, &gotBody +} + +func newTestClients(t *testing.T, baseURL string) GetFlashdutyClientFn { + t.Helper() + client, err := flashduty.NewClient("test-key", flashduty.WithBaseURL(baseURL)) + if err != nil { + t.Fatalf("new go-flashduty client: %v", err) + } + return func(ctx context.Context) (context.Context, *Clients, error) { + return ctx, &Clients{New: client}, nil + } +} + +func callOK(t *testing.T, handler func(context.Context, mcp.CallToolRequest) (*mcp.CallToolResult, error), name string, args map[string]any) { + t.Helper() + result, err := handler(context.Background(), mcp.CallToolRequest{ + Params: mcp.CallToolParams{Name: name, Arguments: args}, + }) + if err != nil { + t.Fatalf("handler returned error: %v", err) + } + if result.IsError { + txt, _ := mcp.AsTextContent(result.Content[0]) + t.Fatalf("expected success result, got error: %s", txt.Text) + } +} + +// TestQueryIncidentsPageReachesWire verifies `page` is sent to /incident/list as +// the 1-based `p` field, and that the default (page 1) omits it so non-paginating +// callers keep an unchanged payload. +func TestQueryIncidentsPageReachesWire(t *testing.T) { + t.Parallel() + + t.Run("page 2 sends p", func(t *testing.T) { + ts, gotPath, gotBody := captureWire(t) + defer ts.Close() + _, handler := QueryIncidents(newTestClients(t, ts.URL), translations.NullTranslationHelper) + + callOK(t, handler, "query_incidents", map[string]any{ + "since": "7d", "until": "now", "page": float64(2), + }) + + if *gotPath != "/incident/list" { + t.Fatalf("path = %q, want /incident/list", *gotPath) + } + if p, ok := (*gotBody)["p"]; !ok || p != float64(2) { + t.Errorf("p = %#v, want 2", (*gotBody)["p"]) + } + }) + + t.Run("default omits p", func(t *testing.T) { + ts, _, gotBody := captureWire(t) + defer ts.Close() + _, handler := QueryIncidents(newTestClients(t, ts.URL), translations.NullTranslationHelper) + + callOK(t, handler, "query_incidents", map[string]any{"since": "7d", "until": "now"}) + + if _, ok := (*gotBody)["p"]; ok { + t.Errorf("p should be absent for default page 1, got %#v", (*gotBody)["p"]) + } + }) +} + +// TestQueryChannelsLimitPageReachesWire verifies the newly added limit/page reach +// /channel/list. channel_ids is a filter on the same paginated endpoint (not a +// dedicated by-ID lookup), so a channel_ids lookup pages like any other listing. +func TestQueryChannelsLimitPageReachesWire(t *testing.T) { + t.Parallel() + + t.Run("search path sends limit and p", func(t *testing.T) { + ts, gotPath, gotBody := captureWire(t) + defer ts.Close() + _, handler := QueryChannels(newTestClients(t, ts.URL), translations.NullTranslationHelper) + + callOK(t, handler, "query_channels", map[string]any{ + "name": "prod", "limit": float64(50), "page": float64(3), + }) + + if *gotPath != "/channel/list" { + t.Fatalf("path = %q, want /channel/list", *gotPath) + } + if v, ok := (*gotBody)["limit"]; !ok || v != float64(50) { + t.Errorf("limit = %#v, want 50", (*gotBody)["limit"]) + } + if v, ok := (*gotBody)["p"]; !ok || v != float64(3) { + t.Errorf("p = %#v, want 3", (*gotBody)["p"]) + } + }) + + t.Run("channel_ids lookup also pages", func(t *testing.T) { + ts, gotPath, gotBody := captureWire(t) + defer ts.Close() + _, handler := QueryChannels(newTestClients(t, ts.URL), translations.NullTranslationHelper) + + callOK(t, handler, "query_channels", map[string]any{ + "channel_ids": "1,2,3", "limit": float64(50), "page": float64(3), + }) + + if *gotPath != "/channel/list" { + t.Fatalf("path = %q, want /channel/list", *gotPath) + } + if v, ok := (*gotBody)["limit"]; !ok || v != float64(50) { + t.Errorf("limit = %#v, want 50 (channel_ids lookup must page the same endpoint)", (*gotBody)["limit"]) + } + if v, ok := (*gotBody)["p"]; !ok || v != float64(3) { + t.Errorf("p = %#v, want 3", (*gotBody)["p"]) + } + }) +} + +// TestQueryMembersPagingReachesWire verifies the search path pages via +// /member/list, while a person_ids lookup uses the dedicated /member/infos +// endpoint and carries no paging fields. +func TestQueryMembersPagingReachesWire(t *testing.T) { + t.Parallel() + + t.Run("name search sends limit and p", func(t *testing.T) { + ts, gotPath, gotBody := captureWire(t) + defer ts.Close() + _, handler := QueryMembers(newTestClients(t, ts.URL), translations.NullTranslationHelper) + + callOK(t, handler, "query_members", map[string]any{ + "name": "alice", "limit": float64(40), "page": float64(2), + }) + + if *gotPath != "/member/list" { + t.Fatalf("path = %q, want /member/list", *gotPath) + } + if v, ok := (*gotBody)["limit"]; !ok || v != float64(40) { + t.Errorf("limit = %#v, want 40", (*gotBody)["limit"]) + } + if v, ok := (*gotBody)["p"]; !ok || v != float64(2) { + t.Errorf("p = %#v, want 2", (*gotBody)["p"]) + } + }) + + t.Run("person_ids lookup is unpaged", func(t *testing.T) { + ts, gotPath, gotBody := captureWire(t) + defer ts.Close() + _, handler := QueryMembers(newTestClients(t, ts.URL), translations.NullTranslationHelper) + + callOK(t, handler, "query_members", map[string]any{ + "person_ids": "1,2", "limit": float64(40), "page": float64(2), + }) + + // Direct ID lookup uses the dedicated /person/infos endpoint, which + // returns the requested profiles without paging. + if !strings.Contains(*gotPath, "/infos") { + t.Fatalf("path = %q, want the /infos by-ID endpoint", *gotPath) + } + if _, ok := (*gotBody)["p"]; ok { + t.Errorf("p should be absent for person_ids lookup, got %#v", (*gotBody)["p"]) + } + if _, ok := (*gotBody)["limit"]; ok { + t.Errorf("limit should be absent for person_ids lookup, got %#v", (*gotBody)["limit"]) + } + }) +} + +// decodeResult unmarshals a tool result's JSON text payload into a map. +func decodeResult(t *testing.T, result *mcp.CallToolResult) map[string]any { + t.Helper() + txt, ok := mcp.AsTextContent(result.Content[0]) + if !ok { + t.Fatalf("expected text content, got %T", result.Content[0]) + } + var m map[string]any + if err := json.Unmarshal([]byte(txt.Text), &m); err != nil { + t.Fatalf("decode result JSON: %v (raw: %s)", err, txt.Text) + } + return m +} + +// TestQueryChangesPaging verifies the normal path pages via /change/list, and +// that a change_ids lookup (a client-side filter over one page — /change/list +// has no server-side id filter) does NOT emit a misleading page hint keyed off +// the window's unfiltered total. +func TestQueryChangesPaging(t *testing.T) { + t.Parallel() + + t.Run("normal path sends limit and p", func(t *testing.T) { + ts, gotPath, gotBody := captureWire(t) + defer ts.Close() + _, handler := QueryChanges(newTestClients(t, ts.URL), translations.NullTranslationHelper) + + callOK(t, handler, "query_changes", map[string]any{ + "since": "7d", "until": "now", "limit": float64(30), "page": float64(2), + }) + + if *gotPath != "/change/list" { + t.Fatalf("path = %q, want /change/list", *gotPath) + } + if v, ok := (*gotBody)["limit"]; !ok || v != float64(30) { + t.Errorf("limit = %#v, want 30", (*gotBody)["limit"]) + } + if v, ok := (*gotBody)["p"]; !ok || v != float64(2) { + t.Errorf("p = %#v, want 2", (*gotBody)["p"]) + } + }) + + t.Run("change_ids lookup reports found count, no page hint", func(t *testing.T) { + // Backend returns 2 changes on the page but claims a huge window total. + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(map[string]any{ + "data": map[string]any{ + "items": []any{ + map[string]any{"change_id": "c1"}, + map[string]any{"change_id": "c2"}, + }, + "total": 5000, + }, + }) + })) + defer ts.Close() + _, handler := QueryChanges(newTestClients(t, ts.URL), translations.NullTranslationHelper) + + result, err := handler(context.Background(), mcp.CallToolRequest{ + Params: mcp.CallToolParams{Name: "query_changes", Arguments: map[string]any{ + "since": "7d", "until": "now", "change_ids": "c1", + }}, + }) + if err != nil || result.IsError { + t.Fatalf("handler failed: err=%v isErr=%v", err, result.IsError) + } + payload := decodeResult(t, result) + if _, ok := payload["truncated"]; ok { + t.Errorf("change_ids lookup must not set truncated, got %#v", payload["truncated"]) + } + if payload["total"] != float64(1) { + t.Errorf("total = %#v, want 1 (found count, not the window's 5000)", payload["total"]) + } + if changes, _ := payload["changes"].([]any); len(changes) != 1 { + t.Errorf("changes = %#v, want exactly the one matched id", payload["changes"]) + } + }) +} + +// TestAddPageHint locks the pagination-aware truncation logic: the hint must +// fire only when the caller has NOT paged through everything. The critical case +// is the final partial page — comparing total>count alone would wrongly flag it +// and send the agent chasing an empty next page. +func TestAddPageHint(t *testing.T) { + t.Parallel() + + cases := []struct { + name string + count, total int + page, limit int + wantTruncated bool + wantNextPageToken string // substring the hint must contain when truncated + }{ + {"first page of many", 20, 25, 1, 20, true, "page:2"}, + {"last partial page", 5, 25, 2, 20, false, ""}, + {"single full page", 5, 5, 1, 20, false, ""}, + {"middle page", 20, 100, 3, 20, true, "page:4"}, + {"exact last full page", 20, 40, 2, 20, false, ""}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + res := addPageHint(map[string]any{}, tc.count, tc.total, tc.page, tc.limit) + _, truncated := res["truncated"] + if truncated != tc.wantTruncated { + t.Fatalf("truncated = %v, want %v (count=%d total=%d page=%d limit=%d)", + truncated, tc.wantTruncated, tc.count, tc.total, tc.page, tc.limit) + } + if tc.wantTruncated { + hint, _ := res["hint"].(string) + if !strings.Contains(hint, tc.wantNextPageToken) { + t.Errorf("hint = %q, want it to mention %q", hint, tc.wantNextPageToken) + } + } + }) + } +} diff --git a/pkg/flashduty/users.go b/pkg/flashduty/users.go index b8d418c..34351a3 100644 --- a/pkg/flashduty/users.go +++ b/pkg/flashduty/users.go @@ -24,6 +24,8 @@ func QueryMembers(getClient GetFlashdutyClientFn, t translations.TranslationHelp mcp.WithString("person_ids", mcp.Description("Comma-separated person IDs for direct lookup.")), mcp.WithString("name", mcp.Description("Search by member name (fuzzy match).")), mcp.WithString("email", mcp.Description("Search by email address.")), + mcp.WithNumber("limit", mcp.Description(LimitDescription), mcp.DefaultNumber(20), mcp.Min(1), mcp.Max(100)), + mcp.WithNumber("page", mcp.Description(PageDescription), mcp.DefaultNumber(1), mcp.Min(1)), ), func(ctx context.Context, request mcp.CallToolRequest) (*mcp.CallToolResult, error) { ctx, client, err := getClient(ctx) if err != nil { @@ -33,6 +35,7 @@ func QueryMembers(getClient GetFlashdutyClientFn, t translations.TranslationHelp personIdsStr, _ := OptionalParam[string](request, "person_ids") name, _ := OptionalParam[string](request, "name") email, _ := OptionalParam[string](request, "email") + limit, page := optionalPaging(request, defaultQueryLimit) // Direct ID lookup uses /member/infos (PersonInfos), which returns // profiles without a separate total. @@ -66,16 +69,21 @@ func QueryMembers(getClient GetFlashdutyClientFn, t translations.TranslationHelp if query == "" { query = email } - out, _, err := client.New.Members.MemberList(ctx, &flashduty.MemberListRequest{Query: query}) + memberReq := &flashduty.MemberListRequest{Query: query} + memberReq.Limit = limit + if page > 1 { + memberReq.Page = page + } + out, _, err := client.New.Members.MemberList(ctx, memberReq) if err != nil { return mcp.NewToolResultError(fmt.Sprintf("Unable to retrieve members: %v", err)), nil } total := int(out.Total) - return MarshalResult(addTruncationHint(map[string]any{ + return MarshalResult(addPageHint(map[string]any{ "members": out.Items, "total": total, - }, len(out.Items), total)), nil + }, len(out.Items), total, page, limit)), nil } } @@ -91,6 +99,8 @@ func QueryTeams(getClient GetFlashdutyClientFn, t translations.TranslationHelper }), mcp.WithString("team_ids", mcp.Description("Comma-separated team IDs for direct lookup.")), mcp.WithString("name", mcp.Description("Search by team name.")), + mcp.WithNumber("limit", mcp.Description(LimitDescription), mcp.DefaultNumber(20), mcp.Min(1), mcp.Max(100)), + mcp.WithNumber("page", mcp.Description(PageDescription), mcp.DefaultNumber(1), mcp.Min(1)), ), func(ctx context.Context, request mcp.CallToolRequest) (*mcp.CallToolResult, error) { ctx, client, err := getClient(ctx) if err != nil { @@ -99,6 +109,7 @@ func QueryTeams(getClient GetFlashdutyClientFn, t translations.TranslationHelper teamIdsStr, _ := OptionalParam[string](request, "team_ids") name, _ := OptionalParam[string](request, "name") + limit, page := optionalPaging(request, defaultQueryLimit) // Direct ID lookup uses /team/infos (ReadInfos) and preserves the // historical `items`-only response shape. @@ -123,15 +134,20 @@ func QueryTeams(getClient GetFlashdutyClientFn, t translations.TranslationHelper }), nil } - out, _, err := client.New.Teams.ReadList(ctx, &flashduty.TeamListRequest{Query: name}) + teamReq := &flashduty.TeamListRequest{Query: name} + teamReq.Limit = limit + if page > 1 { + teamReq.Page = page + } + out, _, err := client.New.Teams.ReadList(ctx, teamReq) if err != nil { return mcp.NewToolResultError(fmt.Sprintf("Unable to retrieve teams: %v", err)), nil } total := int(out.Total) - return MarshalResult(addTruncationHint(map[string]any{ + return MarshalResult(addPageHint(map[string]any{ "teams": out.Items, "total": total, - }, len(out.Items), total)), nil + }, len(out.Items), total, page, limit)), nil } }