From 048d0329518a629cd8813afa8858b7f0e3394ad2 Mon Sep 17 00:00:00 2001 From: Corey Christous Date: Tue, 21 Apr 2026 14:12:40 -0700 Subject: [PATCH 1/3] fix: paginate sem get tasks to return all tasks for a project ListTasks previously issued a single GET /api/v1alpha/tasks and returned whatever the first page contained (~30 tasks), silently discarding the rest. Projects with >30 tasks were invisible beyond the first page. Rewrite ListTasks to loop with page_size=200, following the API's Link: rel="next" header until exhausted, and aggregating all pages. Mirrors the pattern established for pipelines in #247, reusing retry.RetryWithMaxFailures, retry.NonRetryable, and hasNextPage. Includes a safety cap of 500 pages (100k tasks) and nil-header guard. No CLI surface changes -- sem get tasks -p just works now. Existing httpmock responders that matched task URLs exactly are switched to regexp matchers to accommodate the new page/page_size query params. A new Test__ListTasks__MultiPage verifies both pages are fetched via Link: rel=next and aggregated into the output. Reported by Kumar Utkarsh against #246. --- api/client/tasks_v1_alpha.go | 76 +++++++++++++++++++++++++++++++----- cmd/get_tasks_test.go | 53 +++++++++++++++++++++++-- 2 files changed, 116 insertions(+), 13 deletions(-) diff --git a/api/client/tasks_v1_alpha.go b/api/client/tasks_v1_alpha.go index 77b2293..061edc1 100644 --- a/api/client/tasks_v1_alpha.go +++ b/api/client/tasks_v1_alpha.go @@ -3,9 +3,11 @@ package client import ( "errors" "fmt" + "net/http" "net/url" models "github.com/semaphoreci/cli/api/models" + retry "github.com/semaphoreci/cli/api/retry" ) type TasksApiV1AlphaApi struct { @@ -25,21 +27,75 @@ func NewTasksV1AlphaApi() TasksApiV1AlphaApi { } } +// maxTaskPages caps pagination depth to prevent runaway loops; +// at 200 items/page this allows up to 100k tasks per project. +var maxTaskPages = 500 + +// ListTasks fetches all tasks for a project using pagination and aggregates them. func (c *TasksApiV1AlphaApi) ListTasks(projectID string) (models.TaskListV1Alpha, error) { query := url.Values{} query.Add("project_id", projectID) - - body, status, _, err := c.BaseClient.ListWithParams(c.ResourceNamePlural, query) - - if err != nil { - return nil, errors.New(fmt.Sprintf("connecting to Semaphore failed '%s'", err)) - } - - if status != 200 { - return nil, errors.New(fmt.Sprintf("http status %d with message \"%s\" received from upstream", status, body)) + query.Add("page_size", "200") + + allTasks := make(models.TaskListV1Alpha, 0) + currentPage := 1 + const maxFailures = 5 + + for { + query.Set("page", fmt.Sprintf("%d", currentPage)) + + var page models.TaskListV1Alpha + var headers http.Header + + err := retry.RetryWithMaxFailures(maxFailures, func() error { + page = nil + headers = nil + + body, status, hdrs, err := c.BaseClient.ListWithParams(c.ResourceNamePlural, query) + headers = hdrs + if err != nil { + return fmt.Errorf("connecting to Semaphore failed: %w", err) + } + if status != http.StatusOK { + msg := string(body) + if len(msg) > 200 { + msg = msg[:200] + "...(truncated)" + } + httpErr := fmt.Errorf("http status %d with message \"%s\" received from upstream", status, msg) + if status >= 300 && status < 500 && status != http.StatusTooManyRequests { + return retry.NonRetryable(httpErr) + } + return httpErr + } + + pageList, err := models.NewTaskListV1AlphaFromJSON(body) + if err != nil { + return retry.NonRetryable(fmt.Errorf("failed to deserialize tasks list: %w", err)) + } + page = pageList + return nil + }) + + if err != nil { + return nil, fmt.Errorf("failed fetching page %d (after accumulating %d tasks from %d pages): %w", + currentPage, len(allTasks), currentPage-1, err) + } + + allTasks = append(allTasks, page...) + + if headers == nil { + return nil, fmt.Errorf("internal error: response headers missing after fetching page %d", currentPage) + } + if !hasNextPage(headers) { + break + } + if currentPage >= maxTaskPages { + return nil, fmt.Errorf("pagination safety limit reached (%d pages); results may be incomplete -- please narrow your query", maxTaskPages) + } + currentPage++ } - return models.NewTaskListV1AlphaFromJSON(body) + return allTasks, nil } func (c *TasksApiV1AlphaApi) DescribeTask(id string) (*models.TaskDescribeV1Alpha, error) { diff --git a/cmd/get_tasks_test.go b/cmd/get_tasks_test.go index 8215d09..cd19214 100644 --- a/cmd/get_tasks_test.go +++ b/cmd/get_tasks_test.go @@ -2,8 +2,10 @@ package cmd import ( "net/http" + "regexp" "testing" + client "github.com/semaphoreci/cli/api/client" httpmock "github.com/jarcoal/httpmock" "github.com/stretchr/testify/assert" ) @@ -25,7 +27,8 @@ func Test__ListTasks__Response200(t *testing.T) { }, ) - httpmock.RegisterResponder("GET", "https://org.semaphoretext.xyz/api/v1alpha/tasks?project_id=758cb945-7495-4e40-a9a1-4b3991c6a8fe", + httpmock.RegisterRegexpResponder("GET", + regexp.MustCompile(`https://org\.semaphoretext\.xyz/api/v1alpha/tasks\?.*project_id=758cb945-7495-4e40-a9a1-4b3991c6a8fe`), func(req *http.Request) (*http.Response, error) { received = true @@ -68,7 +71,8 @@ func Test__ListTasks__WithProjectID(t *testing.T) { received := false - httpmock.RegisterResponder("GET", "https://org.semaphoretext.xyz/api/v1alpha/tasks?project_id=758cb945-7495-4e40-a9a1-4b3991c6a8fe", + httpmock.RegisterRegexpResponder("GET", + regexp.MustCompile(`https://org\.semaphoretext\.xyz/api/v1alpha/tasks\?.*project_id=758cb945-7495-4e40-a9a1-4b3991c6a8fe`), func(req *http.Request) (*http.Response, error) { received = true @@ -88,7 +92,8 @@ func Test__ListTasks__SuspendedTask(t *testing.T) { received := false - httpmock.RegisterResponder("GET", "https://org.semaphoretext.xyz/api/v1alpha/tasks?project_id=758cb945-7495-4e40-a9a1-4b3991c6a8fe", + httpmock.RegisterRegexpResponder("GET", + regexp.MustCompile(`https://org\.semaphoretext\.xyz/api/v1alpha/tasks\?.*project_id=758cb945-7495-4e40-a9a1-4b3991c6a8fe`), func(req *http.Request) (*http.Response, error) { received = true @@ -212,3 +217,45 @@ func Test__GetTasks__TaskAlias(t *testing.T) { assert.True(t, received, "Expected the 'task' alias to work for describe") } + +func Test__ListTasks__MultiPage(t *testing.T) { + httpmock.Activate() + defer httpmock.DeactivateAndReset() + + page1Received := false + page2Received := false + + httpmock.RegisterRegexpResponder("GET", + regexp.MustCompile(`https://org\.semaphoretext\.xyz/api/v1alpha/tasks\?.*page=1.*`), + func(req *http.Request) (*http.Response, error) { + page1Received = true + body := `[ + {"id": "11111111-1111-1111-1111-111111111111", "name": "t1", "project_id": "758cb945-7495-4e40-a9a1-4b3991c6a8fe", "branch": "main", "pipeline_file": ".semaphore/t1.yml", "recurring": false} + ]` + resp := httpmock.NewStringResponse(200, body) + resp.Header.Set("Link", `; rel="next"`) + return resp, nil + }, + ) + + httpmock.RegisterRegexpResponder("GET", + regexp.MustCompile(`https://org\.semaphoretext\.xyz/api/v1alpha/tasks\?.*page=2.*`), + func(req *http.Request) (*http.Response, error) { + page2Received = true + body := `[ + {"id": "22222222-2222-2222-2222-222222222222", "name": "t2", "project_id": "758cb945-7495-4e40-a9a1-4b3991c6a8fe", "branch": "main", "pipeline_file": ".semaphore/t2.yml", "recurring": false} + ]` + return httpmock.NewStringResponse(200, body), nil + }, + ) + + c := client.NewTasksV1AlphaApi() + tasks, err := c.ListTasks("758cb945-7495-4e40-a9a1-4b3991c6a8fe") + + assert.NoError(t, err) + assert.True(t, page1Received, "Expected page 1 to be fetched") + assert.True(t, page2Received, "Expected page 2 to be fetched (pagination must follow Link: rel=next)") + assert.Len(t, tasks, 2, "Expected tasks from both pages to be aggregated") + assert.Equal(t, "t1", tasks[0].Name, "Expected first task from page 1") + assert.Equal(t, "t2", tasks[1].Name, "Expected second task from page 2") +} From 56606da8973b627f17c503cebaf2ded10fadeea8 Mon Sep 17 00:00:00 2001 From: Corey Christous Date: Tue, 21 Apr 2026 14:30:28 -0700 Subject: [PATCH 2/3] fix: address PR review feedback for task list pagination - maxTaskPages is now a local const inside ListTasks (matches pipelines pattern); the previous package-level var was silently mutable and unnecessary since no test overrides it. - Nil-header guard now fires before the page is appended to allTasks, so on error we don't discard partial state that was already mutated. The error message now includes the accumulated task count for debuggability. - Multi-page test regexps tightened: `page=1.*` would also match page=10, page=11, etc. Replaced with anchored boundary patterns `[?&]page=N(?:&|$)` so the regex cannot accidentally match a later page number if the test is extended. --- api/client/tasks_v1_alpha.go | 14 +++++++------- cmd/get_tasks_test.go | 4 ++-- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/api/client/tasks_v1_alpha.go b/api/client/tasks_v1_alpha.go index 061edc1..074677d 100644 --- a/api/client/tasks_v1_alpha.go +++ b/api/client/tasks_v1_alpha.go @@ -27,10 +27,6 @@ func NewTasksV1AlphaApi() TasksApiV1AlphaApi { } } -// maxTaskPages caps pagination depth to prevent runaway loops; -// at 200 items/page this allows up to 100k tasks per project. -var maxTaskPages = 500 - // ListTasks fetches all tasks for a project using pagination and aggregates them. func (c *TasksApiV1AlphaApi) ListTasks(projectID string) (models.TaskListV1Alpha, error) { query := url.Values{} @@ -40,6 +36,9 @@ func (c *TasksApiV1AlphaApi) ListTasks(projectID string) (models.TaskListV1Alpha allTasks := make(models.TaskListV1Alpha, 0) currentPage := 1 const maxFailures = 5 + // maxTaskPages caps pagination depth to prevent runaway loops; + // at 200 items/page this allows up to 100k tasks per project. + const maxTaskPages = 500 for { query.Set("page", fmt.Sprintf("%d", currentPage)) @@ -81,11 +80,12 @@ func (c *TasksApiV1AlphaApi) ListTasks(projectID string) (models.TaskListV1Alpha currentPage, len(allTasks), currentPage-1, err) } - allTasks = append(allTasks, page...) - if headers == nil { - return nil, fmt.Errorf("internal error: response headers missing after fetching page %d", currentPage) + return nil, fmt.Errorf("internal error: response headers missing after fetching page %d (accumulated %d tasks)", currentPage, len(allTasks)) } + + allTasks = append(allTasks, page...) + if !hasNextPage(headers) { break } diff --git a/cmd/get_tasks_test.go b/cmd/get_tasks_test.go index cd19214..27af8cf 100644 --- a/cmd/get_tasks_test.go +++ b/cmd/get_tasks_test.go @@ -226,7 +226,7 @@ func Test__ListTasks__MultiPage(t *testing.T) { page2Received := false httpmock.RegisterRegexpResponder("GET", - regexp.MustCompile(`https://org\.semaphoretext\.xyz/api/v1alpha/tasks\?.*page=1.*`), + regexp.MustCompile(`https://org\.semaphoretext\.xyz/api/v1alpha/tasks.*[?&]page=1(?:&|$)`), func(req *http.Request) (*http.Response, error) { page1Received = true body := `[ @@ -239,7 +239,7 @@ func Test__ListTasks__MultiPage(t *testing.T) { ) httpmock.RegisterRegexpResponder("GET", - regexp.MustCompile(`https://org\.semaphoretext\.xyz/api/v1alpha/tasks\?.*page=2.*`), + regexp.MustCompile(`https://org\.semaphoretext\.xyz/api/v1alpha/tasks.*[?&]page=2(?:&|$)`), func(req *http.Request) (*http.Response, error) { page2Received = true body := `[ From 3882cd0925c1e197ca97fe12f1ed2b6690c5e606 Mon Sep 17 00:00:00 2001 From: Corey Christous Date: Sat, 25 Apr 2026 13:05:40 -0400 Subject: [PATCH 3/3] test: assert exact request count in multi-page test Without this assertion, a regression in hasNextPage or the break condition could let the loop run extra requests; the existing page1Received/page2Received flags only verify lower bounds. With GetTotalCallCount we explicitly assert the loop terminated when Link: rel=next was absent on page 2. --- cmd/get_tasks_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/cmd/get_tasks_test.go b/cmd/get_tasks_test.go index 27af8cf..cadf45d 100644 --- a/cmd/get_tasks_test.go +++ b/cmd/get_tasks_test.go @@ -258,4 +258,5 @@ func Test__ListTasks__MultiPage(t *testing.T) { assert.Len(t, tasks, 2, "Expected tasks from both pages to be aggregated") assert.Equal(t, "t1", tasks[0].Name, "Expected first task from page 1") assert.Equal(t, "t2", tasks[1].Name, "Expected second task from page 2") + assert.Equal(t, 2, httpmock.GetTotalCallCount(), "Expected exactly two HTTP requests; pagination must stop when Link: rel=next is absent") }