diff --git a/pkg/cmd/agent-task/capi/client.go b/pkg/cmd/agent-task/capi/client.go index 6d35f11c2..c25549121 100644 --- a/pkg/cmd/agent-task/capi/client.go +++ b/pkg/cmd/agent-task/capi/client.go @@ -15,8 +15,7 @@ const capiHost = "api.githubcopilot.com" // CapiClient defines the methods used by the caller. Implementations // may be replaced with test doubles in unit tests. type CapiClient interface { - ListSessionsForViewer(ctx context.Context, limit int) ([]*Session, error) - ListSessionsForRepo(ctx context.Context, owner string, repo string, limit int) ([]*Session, error) + ListLatestSessionsForViewer(ctx context.Context, limit int) ([]*Session, error) CreateJob(ctx context.Context, owner, repo, problemStatement, baseBranch string) (*Job, error) GetJob(ctx context.Context, owner, repo, jobID string) (*Job, error) GetSession(ctx context.Context, id string) (*Session, error) diff --git a/pkg/cmd/agent-task/capi/client_mock.go b/pkg/cmd/agent-task/capi/client_mock.go index fa59ffab7..325a8d513 100644 --- a/pkg/cmd/agent-task/capi/client_mock.go +++ b/pkg/cmd/agent-task/capi/client_mock.go @@ -33,15 +33,12 @@ var _ CapiClient = &CapiClientMock{} // GetSessionLogsFunc: func(ctx context.Context, id string) ([]byte, error) { // panic("mock out the GetSessionLogs method") // }, +// ListLatestSessionsForViewerFunc: func(ctx context.Context, limit int) ([]*Session, error) { +// panic("mock out the ListLatestSessionsForViewer method") +// }, // ListSessionsByResourceIDFunc: func(ctx context.Context, resourceType string, resourceID int64, limit int) ([]*Session, error) { // panic("mock out the ListSessionsByResourceID method") // }, -// ListSessionsForRepoFunc: func(ctx context.Context, owner string, repo string, limit int) ([]*Session, error) { -// panic("mock out the ListSessionsForRepo method") -// }, -// ListSessionsForViewerFunc: func(ctx context.Context, limit int) ([]*Session, error) { -// panic("mock out the ListSessionsForViewer method") -// }, // } // // // use mockedCapiClient in code that requires CapiClient @@ -64,15 +61,12 @@ type CapiClientMock struct { // GetSessionLogsFunc mocks the GetSessionLogs method. GetSessionLogsFunc func(ctx context.Context, id string) ([]byte, error) + // ListLatestSessionsForViewerFunc mocks the ListLatestSessionsForViewer method. + ListLatestSessionsForViewerFunc func(ctx context.Context, limit int) ([]*Session, error) + // ListSessionsByResourceIDFunc mocks the ListSessionsByResourceID method. ListSessionsByResourceIDFunc func(ctx context.Context, resourceType string, resourceID int64, limit int) ([]*Session, error) - // ListSessionsForRepoFunc mocks the ListSessionsForRepo method. - ListSessionsForRepoFunc func(ctx context.Context, owner string, repo string, limit int) ([]*Session, error) - - // ListSessionsForViewerFunc mocks the ListSessionsForViewer method. - ListSessionsForViewerFunc func(ctx context.Context, limit int) ([]*Session, error) - // calls tracks calls to the methods. calls struct { // CreateJob holds details about calls to the CreateJob method. @@ -126,6 +120,13 @@ type CapiClientMock struct { // ID is the id argument value. ID string } + // ListLatestSessionsForViewer holds details about calls to the ListLatestSessionsForViewer method. + ListLatestSessionsForViewer []struct { + // Ctx is the ctx argument value. + Ctx context.Context + // Limit is the limit argument value. + Limit int + } // ListSessionsByResourceID holds details about calls to the ListSessionsByResourceID method. ListSessionsByResourceID []struct { // Ctx is the ctx argument value. @@ -137,33 +138,14 @@ type CapiClientMock struct { // Limit is the limit argument value. Limit int } - // ListSessionsForRepo holds details about calls to the ListSessionsForRepo method. - ListSessionsForRepo []struct { - // Ctx is the ctx argument value. - Ctx context.Context - // Owner is the owner argument value. - Owner string - // Repo is the repo argument value. - Repo string - // Limit is the limit argument value. - Limit int - } - // ListSessionsForViewer holds details about calls to the ListSessionsForViewer method. - ListSessionsForViewer []struct { - // Ctx is the ctx argument value. - Ctx context.Context - // Limit is the limit argument value. - Limit int - } } - lockCreateJob sync.RWMutex - lockGetJob sync.RWMutex - lockGetPullRequestDatabaseID sync.RWMutex - lockGetSession sync.RWMutex - lockGetSessionLogs sync.RWMutex - lockListSessionsByResourceID sync.RWMutex - lockListSessionsForRepo sync.RWMutex - lockListSessionsForViewer sync.RWMutex + lockCreateJob sync.RWMutex + lockGetJob sync.RWMutex + lockGetPullRequestDatabaseID sync.RWMutex + lockGetSession sync.RWMutex + lockGetSessionLogs sync.RWMutex + lockListLatestSessionsForViewer sync.RWMutex + lockListSessionsByResourceID sync.RWMutex } // CreateJob calls CreateJobFunc. @@ -378,6 +360,42 @@ func (mock *CapiClientMock) GetSessionLogsCalls() []struct { return calls } +// ListLatestSessionsForViewer calls ListLatestSessionsForViewerFunc. +func (mock *CapiClientMock) ListLatestSessionsForViewer(ctx context.Context, limit int) ([]*Session, error) { + if mock.ListLatestSessionsForViewerFunc == nil { + panic("CapiClientMock.ListLatestSessionsForViewerFunc: method is nil but CapiClient.ListLatestSessionsForViewer was just called") + } + callInfo := struct { + Ctx context.Context + Limit int + }{ + Ctx: ctx, + Limit: limit, + } + mock.lockListLatestSessionsForViewer.Lock() + mock.calls.ListLatestSessionsForViewer = append(mock.calls.ListLatestSessionsForViewer, callInfo) + mock.lockListLatestSessionsForViewer.Unlock() + return mock.ListLatestSessionsForViewerFunc(ctx, limit) +} + +// ListLatestSessionsForViewerCalls gets all the calls that were made to ListLatestSessionsForViewer. +// Check the length with: +// +// len(mockedCapiClient.ListLatestSessionsForViewerCalls()) +func (mock *CapiClientMock) ListLatestSessionsForViewerCalls() []struct { + Ctx context.Context + Limit int +} { + var calls []struct { + Ctx context.Context + Limit int + } + mock.lockListLatestSessionsForViewer.RLock() + calls = mock.calls.ListLatestSessionsForViewer + mock.lockListLatestSessionsForViewer.RUnlock() + return calls +} + // ListSessionsByResourceID calls ListSessionsByResourceIDFunc. func (mock *CapiClientMock) ListSessionsByResourceID(ctx context.Context, resourceType string, resourceID int64, limit int) ([]*Session, error) { if mock.ListSessionsByResourceIDFunc == nil { @@ -421,83 +439,3 @@ func (mock *CapiClientMock) ListSessionsByResourceIDCalls() []struct { mock.lockListSessionsByResourceID.RUnlock() return calls } - -// ListSessionsForRepo calls ListSessionsForRepoFunc. -func (mock *CapiClientMock) ListSessionsForRepo(ctx context.Context, owner string, repo string, limit int) ([]*Session, error) { - if mock.ListSessionsForRepoFunc == nil { - panic("CapiClientMock.ListSessionsForRepoFunc: method is nil but CapiClient.ListSessionsForRepo was just called") - } - callInfo := struct { - Ctx context.Context - Owner string - Repo string - Limit int - }{ - Ctx: ctx, - Owner: owner, - Repo: repo, - Limit: limit, - } - mock.lockListSessionsForRepo.Lock() - mock.calls.ListSessionsForRepo = append(mock.calls.ListSessionsForRepo, callInfo) - mock.lockListSessionsForRepo.Unlock() - return mock.ListSessionsForRepoFunc(ctx, owner, repo, limit) -} - -// ListSessionsForRepoCalls gets all the calls that were made to ListSessionsForRepo. -// Check the length with: -// -// len(mockedCapiClient.ListSessionsForRepoCalls()) -func (mock *CapiClientMock) ListSessionsForRepoCalls() []struct { - Ctx context.Context - Owner string - Repo string - Limit int -} { - var calls []struct { - Ctx context.Context - Owner string - Repo string - Limit int - } - mock.lockListSessionsForRepo.RLock() - calls = mock.calls.ListSessionsForRepo - mock.lockListSessionsForRepo.RUnlock() - return calls -} - -// ListSessionsForViewer calls ListSessionsForViewerFunc. -func (mock *CapiClientMock) ListSessionsForViewer(ctx context.Context, limit int) ([]*Session, error) { - if mock.ListSessionsForViewerFunc == nil { - panic("CapiClientMock.ListSessionsForViewerFunc: method is nil but CapiClient.ListSessionsForViewer was just called") - } - callInfo := struct { - Ctx context.Context - Limit int - }{ - Ctx: ctx, - Limit: limit, - } - mock.lockListSessionsForViewer.Lock() - mock.calls.ListSessionsForViewer = append(mock.calls.ListSessionsForViewer, callInfo) - mock.lockListSessionsForViewer.Unlock() - return mock.ListSessionsForViewerFunc(ctx, limit) -} - -// ListSessionsForViewerCalls gets all the calls that were made to ListSessionsForViewer. -// Check the length with: -// -// len(mockedCapiClient.ListSessionsForViewerCalls()) -func (mock *CapiClientMock) ListSessionsForViewerCalls() []struct { - Ctx context.Context - Limit int -} { - var calls []struct { - Ctx context.Context - Limit int - } - mock.lockListSessionsForViewer.RLock() - calls = mock.calls.ListSessionsForViewer - mock.lockListSessionsForViewer.RUnlock() - return calls -} diff --git a/pkg/cmd/agent-task/capi/sessions.go b/pkg/cmd/agent-task/capi/sessions.go index f4f12254d..f6825e674 100644 --- a/pkg/cmd/agent-task/capi/sessions.go +++ b/pkg/cmd/agent-task/capi/sessions.go @@ -88,9 +88,9 @@ type Session struct { User *api.GitHubUser } -// ListSessionsForViewer lists all agent sessions for the +// ListLatestSessionsForViewer lists all agent sessions for the // authenticated user up to limit. -func (c *CAPIClient) ListSessionsForViewer(ctx context.Context, limit int) ([]*Session, error) { +func (c *CAPIClient) ListLatestSessionsForViewer(ctx context.Context, limit int) ([]*Session, error) { if limit == 0 { return nil, nil } @@ -99,7 +99,8 @@ func (c *CAPIClient) ListSessionsForViewer(ctx context.Context, limit int) ([]*S pageSize := defaultSessionsPerPage sessions := make([]session, 0, limit+pageSize) - + var seenResources []int64 + var latestSessions []session for page := 1; ; page++ { req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, http.NoBody) if err != nil { @@ -109,6 +110,7 @@ func (c *CAPIClient) ListSessionsForViewer(ctx context.Context, limit int) ([]*S q := req.URL.Query() q.Set("page_size", strconv.Itoa(pageSize)) q.Set("page_number", strconv.Itoa(page)) + q.Set("sort", "last_updated_at,desc") req.URL.RawQuery = q.Encode() res, err := c.httpClient.Do(req) @@ -127,17 +129,30 @@ func (c *CAPIClient) ListSessionsForViewer(ctx context.Context, limit int) ([]*S } sessions = append(sessions, response.Sessions...) - if len(response.Sessions) < pageSize || len(sessions) >= limit { + + // De-duplicate sessions by resource ID. + // Because the API returns newest first, + // we can safely skip any additional sessions + // for a resource we have already seen. + for _, s := range sessions { + if slices.Contains(seenResources, s.ResourceID) { + continue + } + seenResources = append(seenResources, s.ResourceID) + latestSessions = append(latestSessions, s) + } + + if len(response.Sessions) < pageSize || len(latestSessions) >= limit { break } } // Drop any above the limit - if len(sessions) > limit { - sessions = sessions[:limit] + if len(latestSessions) > limit { + latestSessions = latestSessions[:limit] } - result, err := c.hydrateSessionPullRequestsAndUsers(sessions) + result, err := c.hydrateSessionPullRequestsAndUsers(latestSessions) if err != nil { return nil, fmt.Errorf("failed to fetch session resources: %w", err) } @@ -145,65 +160,6 @@ func (c *CAPIClient) ListSessionsForViewer(ctx context.Context, limit int) ([]*S return result, nil } -// ListSessionsForRepo lists agent sessions for a specific repository identified by owner/name up to limit. -func (c *CAPIClient) ListSessionsForRepo(ctx context.Context, owner string, repo string, limit int) ([]*Session, error) { - if owner == "" || repo == "" { - return nil, fmt.Errorf("owner and repo are required") - } - - if limit == 0 { - return nil, nil - } - - url := fmt.Sprintf("%s/agents/sessions/nwo/%s/%s", baseCAPIURL, url.PathEscape(owner), url.PathEscape(repo)) - pageSize := defaultSessionsPerPage - - sessions := make([]session, 0, limit+pageSize) - - for page := 1; ; page++ { - req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, http.NoBody) - if err != nil { - return nil, err - } - - q := req.URL.Query() - q.Set("page_size", strconv.Itoa(pageSize)) - q.Set("page_number", strconv.Itoa(page)) - req.URL.RawQuery = q.Encode() - - res, err := c.httpClient.Do(req) - if err != nil { - return nil, err - } - defer res.Body.Close() - if res.StatusCode != http.StatusOK { - return nil, fmt.Errorf("failed to list sessions: %s", res.Status) - } - var response struct { - Sessions []session `json:"sessions"` - } - if err := json.NewDecoder(res.Body).Decode(&response); err != nil { - return nil, fmt.Errorf("failed to decode sessions response: %w", err) - } - - sessions = append(sessions, response.Sessions...) - if len(response.Sessions) < pageSize || len(sessions) >= limit { - break - } - } - - // Drop any above the limit - if len(sessions) > limit { - sessions = sessions[:limit] - } - - result, err := c.hydrateSessionPullRequestsAndUsers(sessions) - if err != nil { - return nil, fmt.Errorf("failed to fetch session resources: %w", err) - } - return result, nil -} - // GetSession retrieves a specific agent session by ID. func (c *CAPIClient) GetSession(ctx context.Context, id string) (*Session, error) { if id == "" { diff --git a/pkg/cmd/agent-task/capi/sessions_test.go b/pkg/cmd/agent-task/capi/sessions_test.go index 45984dfec..1c49444db 100644 --- a/pkg/cmd/agent-task/capi/sessions_test.go +++ b/pkg/cmd/agent-task/capi/sessions_test.go @@ -514,529 +514,7 @@ func TestListSessionsForViewer(t *testing.T) { }() } - sessions, err := capiClient.ListSessionsForViewer(context.Background(), tt.limit) - - if tt.wantErr != "" { - require.ErrorContains(t, err, tt.wantErr) - require.Nil(t, sessions) - return - } - - require.NoError(t, err) - require.Equal(t, tt.wantOut, sessions) - }) - } -} - -func TestListSessionForRepoRequiresRepo(t *testing.T) { - client := &CAPIClient{} - - _, err := client.ListSessionsForRepo(context.Background(), "", "only-repo", 0) - assert.EqualError(t, err, "owner and repo are required") - _, err = client.ListSessionsForRepo(context.Background(), "only-owner", "", 0) - assert.EqualError(t, err, "owner and repo are required") - _, err = client.ListSessionsForRepo(context.Background(), "", "", 0) - assert.EqualError(t, err, "owner and repo are required") -} - -func TestListSessionsForRepo(t *testing.T) { - sampleDateString := "2025-08-29T00:00:00Z" - sampleDate, err := time.Parse(time.RFC3339, sampleDateString) - require.NoError(t, err) - - tests := []struct { - name string - perPage int - limit int - httpStubs func(*testing.T, *httpmock.Registry) - wantErr string - wantOut []*Session - }{ - { - name: "zero limit", - limit: 0, - wantOut: nil, - }, - { - name: "no sessions", - limit: 10, - httpStubs: func(t *testing.T, reg *httpmock.Registry) { - reg.Register( - httpmock.WithHost( - httpmock.QueryMatcher("GET", "agents/sessions/nwo/OWNER/REPO", url.Values{ - "page_number": {"1"}, - "page_size": {"50"}, - }), - "api.githubcopilot.com", - ), - httpmock.StringResponse(`{"sessions":[]}`), - ) - }, - wantOut: nil, - }, - { - name: "single session", - limit: 10, - httpStubs: func(t *testing.T, reg *httpmock.Registry) { - reg.Register( - httpmock.WithHost( - httpmock.QueryMatcher("GET", "agents/sessions/nwo/OWNER/REPO", url.Values{ - "page_number": {"1"}, - "page_size": {"50"}, - }), - "api.githubcopilot.com", - ), - httpmock.StringResponse(heredoc.Docf(` - { - "sessions": [ - { - "id": "sess1", - "name": "Build artifacts", - "user_id": 1, - "agent_id": 2, - "logs": "", - "state": "completed", - "owner_id": 10, - "repo_id": 1000, - "resource_type": "pull", - "resource_id": 2000, - "created_at": "%[1]s", - "premium_requests": 0.1 - } - ] - }`, - sampleDateString, - )), - ) - // GraphQL hydration - reg.Register( - httpmock.GraphQL(`query FetchPRsAndUsersForAgentTaskSessions\b`), - httpmock.GraphQLQuery(heredoc.Docf(` - { - "data": { - "nodes": [ - { - "__typename": "PullRequest", - "id": "PR_node", - "fullDatabaseId": "2000", - "number": 42, - "title": "Improve docs", - "state": "OPEN", - "isDraft": true, - "url": "https://github.com/OWNER/REPO/pull/42", - "body": "", - "createdAt": "%[1]s", - "updatedAt": "%[1]s", - "repository": { - "nameWithOwner": "OWNER/REPO" - } - }, - { - "__typename": "User", - "login": "octocat", - "name": "Octocat", - "databaseId": 1 - } - ] - } - }`, - sampleDateString, - ), func(q string, vars map[string]interface{}) { - assert.Equal(t, []interface{}{"PR_kwDNA-jNB9A", "U_kgAB"}, vars["ids"]) - }), - ) - }, - wantOut: []*Session{ - { - ID: "sess1", - Name: "Build artifacts", - UserID: 1, - AgentID: 2, - Logs: "", - State: "completed", - OwnerID: 10, - RepoID: 1000, - ResourceType: "pull", - ResourceID: 2000, - CreatedAt: sampleDate, - PremiumRequests: 0.1, - PullRequest: &api.PullRequest{ - ID: "PR_node", - FullDatabaseID: "2000", - Number: 42, - Title: "Improve docs", - State: "OPEN", - IsDraft: true, - URL: "https://github.com/OWNER/REPO/pull/42", - Body: "", - CreatedAt: sampleDate, - UpdatedAt: sampleDate, - Repository: &api.PRRepository{ - NameWithOwner: "OWNER/REPO", - }, - }, - User: &api.GitHubUser{ - Login: "octocat", - Name: "Octocat", - DatabaseID: 1, - }, - }, - }, - }, - { - // This happens at the early moments of a session lifecycle, before a PR is created and associated with it. - name: "single session, no pull request resource", - limit: 10, - httpStubs: func(t *testing.T, reg *httpmock.Registry) { - reg.Register( - httpmock.WithHost( - httpmock.QueryMatcher("GET", "agents/sessions/nwo/OWNER/REPO", url.Values{ - "page_number": {"1"}, - "page_size": {"50"}, - }), - "api.githubcopilot.com", - ), - httpmock.StringResponse(heredoc.Docf(` - { - "sessions": [ - { - "id": "sess1", - "name": "Build artifacts", - "user_id": 1, - "agent_id": 2, - "logs": "", - "state": "completed", - "owner_id": 10, - "repo_id": 1000, - "resource_type": "", - "resource_id": 0, - "created_at": "%[1]s", - "premium_requests": 0.1 - } - ] - }`, - sampleDateString, - )), - ) - // GraphQL hydration - reg.Register( - httpmock.GraphQL(`query FetchPRsAndUsersForAgentTaskSessions\b`), - httpmock.GraphQLQuery(heredoc.Docf(` - { - "data": { - "nodes": [ - { - "__typename": "User", - "login": "octocat", - "name": "Octocat", - "databaseId": 1 - } - ] - } - }`, - sampleDateString, - ), func(q string, vars map[string]interface{}) { - assert.Equal(t, []interface{}{"U_kgAB"}, vars["ids"]) - }), - ) - }, - wantOut: []*Session{ - { - - ID: "sess1", - Name: "Build artifacts", - UserID: 1, - AgentID: 2, - Logs: "", - State: "completed", - OwnerID: 10, - RepoID: 1000, - ResourceType: "", - ResourceID: 0, - CreatedAt: sampleDate, - PremiumRequests: 0.1, - User: &api.GitHubUser{ - Login: "octocat", - Name: "Octocat", - DatabaseID: 1, - }, - }, - }, - }, - { - name: "multiple sessions, paginated", - perPage: 1, // to enforce pagination - limit: 2, - httpStubs: func(t *testing.T, reg *httpmock.Registry) { - reg.Register( - httpmock.WithHost( - httpmock.QueryMatcher("GET", "agents/sessions/nwo/OWNER/REPO", url.Values{ - "page_number": {"1"}, - "page_size": {"1"}, - }), - "api.githubcopilot.com", - ), - httpmock.StringResponse(heredoc.Docf(` - { - "sessions": [ - { - "id": "sess1", - "name": "Build artifacts", - "user_id": 1, - "agent_id": 2, - "logs": "", - "state": "completed", - "owner_id": 10, - "repo_id": 1000, - "resource_type": "pull", - "resource_id": 2000, - "created_at": "%[1]s", - "premium_requests": 0.1 - } - ] - }`, - sampleDateString, - )), - ) - - // Second page - reg.Register( - httpmock.WithHost( - httpmock.QueryMatcher("GET", "agents/sessions/nwo/OWNER/REPO", url.Values{ - "page_number": {"2"}, - "page_size": {"1"}, - }), - "api.githubcopilot.com", - ), - httpmock.StringResponse(heredoc.Docf(` - { - "sessions": [ - { - "id": "sess2", - "name": "Build artifacts", - "user_id": 1, - "agent_id": 2, - "logs": "", - "state": "completed", - "owner_id": 10, - "repo_id": 1000, - "resource_type": "pull", - "resource_id": 2001, - "created_at": "%[1]s", - "premium_requests": 0.1 - } - ] - }`, - sampleDateString, - )), - ) - // GraphQL hydration - reg.Register( - httpmock.GraphQL(`query FetchPRsAndUsersForAgentTaskSessions\b`), - httpmock.GraphQLQuery(heredoc.Docf(` - { - "data": { - "nodes": [ - { - "__typename": "PullRequest", - "id": "PR_node", - "fullDatabaseId": "2000", - "number": 42, - "title": "Improve docs", - "state": "OPEN", - "isDraft": true, - "url": "https://github.com/OWNER/REPO/pull/42", - "body": "", - "createdAt": "%[1]s", - "updatedAt": "%[1]s", - "repository": { - "nameWithOwner": "OWNER/REPO" - } - }, - { - "__typename": "PullRequest", - "id": "PR_node", - "fullDatabaseId": "2001", - "number": 43, - "title": "Improve docs", - "state": "OPEN", - "isDraft": true, - "url": "https://github.com/OWNER/REPO/pull/43", - "body": "", - "createdAt": "%[1]s", - "updatedAt": "%[1]s", - "repository": { - "nameWithOwner": "OWNER/REPO" - } - }, - { - "__typename": "User", - "login": "octocat", - "name": "Octocat", - "databaseId": 1 - } - ] - } - }`, - sampleDateString, - ), func(q string, vars map[string]interface{}) { - assert.Equal(t, []interface{}{"PR_kwDNA-jNB9A", "PR_kwDNA-jNB9E", "U_kgAB"}, vars["ids"]) - }), - ) - }, - wantOut: []*Session{ - { - ID: "sess1", - Name: "Build artifacts", - UserID: 1, - AgentID: 2, - Logs: "", - State: "completed", - OwnerID: 10, - RepoID: 1000, - ResourceType: "pull", - ResourceID: 2000, - CreatedAt: sampleDate, - PremiumRequests: 0.1, - PullRequest: &api.PullRequest{ - ID: "PR_node", - FullDatabaseID: "2000", - Number: 42, - Title: "Improve docs", - State: "OPEN", - IsDraft: true, - URL: "https://github.com/OWNER/REPO/pull/42", - Body: "", - CreatedAt: sampleDate, - UpdatedAt: sampleDate, - Repository: &api.PRRepository{ - NameWithOwner: "OWNER/REPO", - }, - }, - User: &api.GitHubUser{ - Login: "octocat", - Name: "Octocat", - DatabaseID: 1, - }, - }, - { - ID: "sess2", - Name: "Build artifacts", - UserID: 1, - AgentID: 2, - Logs: "", - State: "completed", - OwnerID: 10, - RepoID: 1000, - ResourceType: "pull", - ResourceID: 2001, - CreatedAt: sampleDate, - PremiumRequests: 0.1, - PullRequest: &api.PullRequest{ - ID: "PR_node", - FullDatabaseID: "2001", - Number: 43, - Title: "Improve docs", - State: "OPEN", - IsDraft: true, - URL: "https://github.com/OWNER/REPO/pull/43", - Body: "", - CreatedAt: sampleDate, - UpdatedAt: sampleDate, - Repository: &api.PRRepository{ - NameWithOwner: "OWNER/REPO", - }, - }, - User: &api.GitHubUser{ - Login: "octocat", - Name: "Octocat", - DatabaseID: 1, - }, - }, - }, - }, - { - name: "API error", - limit: 10, - httpStubs: func(t *testing.T, reg *httpmock.Registry) { - reg.Register( - httpmock.WithHost( - httpmock.QueryMatcher("GET", "agents/sessions/nwo/OWNER/REPO", url.Values{ - "page_number": {"1"}, - "page_size": {"50"}, - }), - "api.githubcopilot.com", - ), - httpmock.StatusStringResponse(500, "{}"), - ) - }, - wantErr: "failed to list sessions:", - }, { - name: "API error at hydration", - limit: 10, - httpStubs: func(t *testing.T, reg *httpmock.Registry) { - reg.Register( - httpmock.WithHost( - httpmock.QueryMatcher("GET", "agents/sessions/nwo/OWNER/REPO", url.Values{ - "page_number": {"1"}, - "page_size": {"50"}, - }), - "api.githubcopilot.com", - ), - httpmock.StringResponse(heredoc.Docf(` - { - "sessions": [ - { - "id": "sess1", - "name": "Build artifacts", - "user_id": 1, - "agent_id": 2, - "logs": "", - "state": "completed", - "owner_id": 10, - "repo_id": 1000, - "resource_type": "pull", - "resource_id": 2000, - "created_at": "%[1]s", - "premium_requests": 0.1 - } - ] - }`, - sampleDateString, - )), - ) - // GraphQL hydration - reg.Register( - httpmock.GraphQL(`query FetchPRsAndUsersForAgentTaskSessions\b`), - httpmock.StatusStringResponse(500, `{}`), - ) - }, - wantErr: `failed to fetch session resources: non-200 OK status code:`, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - reg := &httpmock.Registry{} - if tt.httpStubs != nil { - tt.httpStubs(t, reg) - } - defer reg.Verify(t) - - httpClient := &http.Client{Transport: reg} - - cfg := config.NewBlankConfig() - capiClient := NewCAPIClient(httpClient, cfg.Authentication()) - - if tt.perPage != 0 { - last := defaultSessionsPerPage - defaultSessionsPerPage = tt.perPage - defer func() { - defaultSessionsPerPage = last - }() - } - - sessions, err := capiClient.ListSessionsForRepo(context.Background(), "OWNER", "REPO", tt.limit) + sessions, err := capiClient.ListLatestSessionsForViewer(context.Background(), tt.limit) if tt.wantErr != "" { require.ErrorContains(t, err, tt.wantErr) diff --git a/pkg/cmd/agent-task/list/list.go b/pkg/cmd/agent-task/list/list.go index 5886955e6..0eb7fd34a 100644 --- a/pkg/cmd/agent-task/list/list.go +++ b/pkg/cmd/agent-task/list/list.go @@ -56,8 +56,6 @@ func NewCmdList(f *cmdutil.Factory, runF func(*ListOptions) error) *cobra.Comman }, } - cmdutil.EnableRepoOverride(cmd, f) - cmd.Flags().IntVarP(&opts.Limit, "limit", "L", defaultLimit, fmt.Sprintf("Maximum number of agent tasks to fetch (default %d)", defaultLimit)) cmd.Flags().BoolVarP(&opts.Web, "web", "w", false, "Open agent tasks in the browser") @@ -66,10 +64,6 @@ func NewCmdList(f *cmdutil.Factory, runF func(*ListOptions) error) *cobra.Comman func listRun(opts *ListOptions) error { if opts.Web { - // Currently the web GUI does not have a page that supports filtering - // based on repo, so we just open the agents dashboard with no args. - // If that page is ever added in the future, we should route to that - // page instead of the global one when --repo is set. webURL := capi.AgentsHomeURL if opts.IO.IsStdoutTTY() { fmt.Fprintf(opts.IO.ErrOut, "Opening %s in your browser.\n", text.DisplayURL(webURL)) @@ -91,24 +85,11 @@ func listRun(opts *ListOptions) error { var sessions []*capi.Session ctx := context.Background() - var repo ghrepo.Interface - if opts.BaseRepo != nil { - // We swallow this error because when CWD is not a repo and - // the --repo flag is not set, we use the global/user session listing. - repo, _ = opts.BaseRepo() + sessions, err = capiClient.ListLatestSessionsForViewer(ctx, opts.Limit) + if err != nil { + return err } - if repo != nil && repo.RepoOwner() != "" && repo.RepoName() != "" { - sessions, err = capiClient.ListSessionsForRepo(ctx, repo.RepoOwner(), repo.RepoName(), opts.Limit) - if err != nil { - return err - } - } else { - sessions, err = capiClient.ListSessionsForViewer(ctx, opts.Limit) - if err != nil { - return err - } - } opts.IO.StopProgressIndicator() if len(sessions) == 0 { @@ -121,6 +102,12 @@ func listRun(opts *ListOptions) error { fmt.Fprintf(opts.IO.ErrOut, "error starting pager: %v\n", err) } + if opts.IO.IsStdoutTTY() { + count := len(sessions) + header := fmt.Sprintf("Showing %s", text.Pluralize(count, "session")) + fmt.Fprintf(opts.IO.Out, "%s\n\n", header) + } + cs := opts.IO.ColorScheme() tp := tableprinter.New(opts.IO, tableprinter.WithHeader("Session Name", "Pull Request", "Repo", "Session State", "Created")) for _, s := range sessions { diff --git a/pkg/cmd/agent-task/list/list_test.go b/pkg/cmd/agent-task/list/list_test.go index 9b981f28a..b9a750a6d 100644 --- a/pkg/cmd/agent-task/list/list_test.go +++ b/pkg/cmd/agent-task/list/list_test.go @@ -3,7 +3,6 @@ package list import ( "bytes" "context" - "errors" "io" "testing" "time" @@ -11,7 +10,6 @@ import ( "github.com/MakeNowJust/heredoc" "github.com/cli/cli/v2/api" "github.com/cli/cli/v2/internal/browser" - "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/pkg/cmd/agent-task/capi" "github.com/cli/cli/v2/pkg/cmdutil" "github.com/cli/cli/v2/pkg/iostreams" @@ -22,11 +20,10 @@ import ( func TestNewCmdList(t *testing.T) { tests := []struct { - name string - args string - wantOpts ListOptions - wantBaseRepo ghrepo.Interface - wantErr string + name string + args string + wantOpts ListOptions + wantErr string }{ { name: "no arguments", @@ -34,14 +31,6 @@ func TestNewCmdList(t *testing.T) { Limit: defaultLimit, }, }, - { - name: "base repo specified", - args: "--repo OWNER/REPO", - wantOpts: ListOptions{ - Limit: defaultLimit, - }, - wantBaseRepo: ghrepo.New("OWNER", "REPO"), - }, { name: "custom limit", args: "--limit 15", @@ -96,12 +85,6 @@ func TestNewCmdList(t *testing.T) { require.NoError(t, err) assert.Equal(t, tt.wantOpts.Limit, gotOpts.Limit) assert.Equal(t, tt.wantOpts.Web, gotOpts.Web) - - if tt.wantBaseRepo != nil { - baseRepo, err := gotOpts.BaseRepo() - require.NoError(t, err) - assert.True(t, ghrepo.IsSame(tt.wantBaseRepo, baseRepo)) - } }) } } @@ -114,8 +97,6 @@ func Test_listRun(t *testing.T) { name string tty bool capiStubs func(*testing.T, *capi.CapiClientMock) - baseRepo ghrepo.Interface - baseRepoErr error limit int web bool wantOut string @@ -127,7 +108,7 @@ func Test_listRun(t *testing.T) { name: "viewer-scoped no sessions", tty: true, capiStubs: func(t *testing.T, m *capi.CapiClientMock) { - m.ListSessionsForViewerFunc = func(ctx context.Context, limit int) ([]*capi.Session, error) { + m.ListLatestSessionsForViewerFunc = func(ctx context.Context, limit int) ([]*capi.Session, error) { return nil, nil } }, @@ -138,7 +119,7 @@ func Test_listRun(t *testing.T) { tty: true, limit: 999, capiStubs: func(t *testing.T, m *capi.CapiClientMock) { - m.ListSessionsForViewerFunc = func(ctx context.Context, limit int) ([]*capi.Session, error) { + m.ListLatestSessionsForViewerFunc = func(ctx context.Context, limit int) ([]*capi.Session, error) { assert.Equal(t, 999, limit) return nil, nil } @@ -149,7 +130,7 @@ func Test_listRun(t *testing.T) { name: "viewer-scoped single session (tty)", tty: true, capiStubs: func(t *testing.T, m *capi.CapiClientMock) { - m.ListSessionsForViewerFunc = func(ctx context.Context, limit int) ([]*capi.Session, error) { + m.ListLatestSessionsForViewerFunc = func(ctx context.Context, limit int) ([]*capi.Session, error) { return []*capi.Session{ { ID: "id1", @@ -168,6 +149,8 @@ func Test_listRun(t *testing.T) { } }, wantOut: heredoc.Doc(` + Showing 1 session + SESSION NAME PULL REQUEST REPO SESSION STATE CREATED s1 #101 OWNER/REPO completed about 6 hours ago `), @@ -176,7 +159,7 @@ func Test_listRun(t *testing.T) { name: "viewer-scoped single session (nontty)", tty: false, capiStubs: func(t *testing.T, m *capi.CapiClientMock) { - m.ListSessionsForViewerFunc = func(ctx context.Context, limit int) ([]*capi.Session, error) { + m.ListLatestSessionsForViewerFunc = func(ctx context.Context, limit int) ([]*capi.Session, error) { return []*capi.Session{ { ID: "id1", @@ -200,7 +183,7 @@ func Test_listRun(t *testing.T) { name: "viewer-scoped many sessions (tty)", tty: true, capiStubs: func(t *testing.T, m *capi.CapiClientMock) { - m.ListSessionsForViewerFunc = func(ctx context.Context, limit int) ([]*capi.Session, error) { + m.ListLatestSessionsForViewerFunc = func(ctx context.Context, limit int) ([]*capi.Session, error) { return []*capi.Session{ { ID: "id1", @@ -284,6 +267,8 @@ func Test_listRun(t *testing.T) { } }, wantOut: heredoc.Doc(` + Showing 6 sessions + SESSION NAME PULL REQUEST REPO SESSION STATE CREATED s1 #101 OWNER/REPO completed about 6 hours ago s2 #102 OWNER/REPO failed about 6 hours ago @@ -293,195 +278,6 @@ func Test_listRun(t *testing.T) { s6 #106 OWNER/REPO mystery about 6 hours ago `), }, - { - name: "repo-scoped no sessions", - tty: true, - baseRepo: ghrepo.New("OWNER", "REPO"), - capiStubs: func(t *testing.T, m *capi.CapiClientMock) { - m.ListSessionsForRepoFunc = func(ctx context.Context, owner, repo string, limit int) ([]*capi.Session, error) { - return nil, nil - } - }, - wantErr: cmdutil.NewNoResultsError("no agent tasks found"), - }, - { - name: "repo-scoped respects --limit/--repo", - tty: true, - limit: 999, - baseRepo: ghrepo.New("OWNER", "REPO"), - capiStubs: func(t *testing.T, m *capi.CapiClientMock) { - m.ListSessionsForRepoFunc = func(ctx context.Context, owner, repo string, limit int) ([]*capi.Session, error) { - assert.Equal(t, 999, limit) - assert.Equal(t, "OWNER", owner) - assert.Equal(t, "REPO", repo) - return nil, nil - } - }, - wantErr: cmdutil.NewNoResultsError("no agent tasks found"), // not important - }, - { - name: "repo-scoped single session (tty)", - tty: true, - baseRepo: ghrepo.New("OWNER", "REPO"), - capiStubs: func(t *testing.T, m *capi.CapiClientMock) { - m.ListSessionsForRepoFunc = func(ctx context.Context, owner, repo string, limit int) ([]*capi.Session, error) { - return []*capi.Session{ - { - ID: "id1", - Name: "s1", - State: "completed", - CreatedAt: sampleDate, - ResourceType: "pull", - PullRequest: &api.PullRequest{ - Number: 101, - Repository: &api.PRRepository{ - NameWithOwner: "OWNER/REPO", - }, - }, - }, - }, nil - } - }, - wantOut: heredoc.Doc(` - SESSION NAME PULL REQUEST REPO SESSION STATE CREATED - s1 #101 OWNER/REPO completed about 6 hours ago - `), - }, - { - name: "repo-scoped single session (nontty)", - tty: false, - baseRepo: ghrepo.New("OWNER", "REPO"), - capiStubs: func(t *testing.T, m *capi.CapiClientMock) { - m.ListSessionsForRepoFunc = func(ctx context.Context, owner, repo string, limit int) ([]*capi.Session, error) { - return []*capi.Session{ - { - ID: "id1", - Name: "s1", - State: "completed", - ResourceType: "pull", - CreatedAt: sampleDate, - PullRequest: &api.PullRequest{ - Number: 101, - Repository: &api.PRRepository{ - NameWithOwner: "OWNER/REPO", - }, - }, - }, - }, nil - } - }, - wantOut: "s1\t#101\tOWNER/REPO\tcompleted\t" + sampleDateString + "\n", // header omitted for non-tty - }, - { - name: "repo-scoped many sessions (tty)", - tty: true, - baseRepo: ghrepo.New("OWNER", "REPO"), - capiStubs: func(t *testing.T, m *capi.CapiClientMock) { - m.ListSessionsForRepoFunc = func(ctx context.Context, owner, repo string, limit int) ([]*capi.Session, error) { - return []*capi.Session{ - { - ID: "id1", - Name: "s1", - State: "completed", - CreatedAt: sampleDate, - ResourceType: "pull", - PullRequest: &api.PullRequest{ - Number: 101, - Repository: &api.PRRepository{ - NameWithOwner: "OWNER/REPO", - }, - }, - }, - { - ID: "id2", - Name: "s2", - State: "failed", - CreatedAt: sampleDate, - ResourceType: "pull", - PullRequest: &api.PullRequest{ - Number: 102, - Repository: &api.PRRepository{ - NameWithOwner: "OWNER/REPO", - }, - }, - }, - { - ID: "id3", - Name: "s3", - State: "in_progress", - CreatedAt: sampleDate, - ResourceType: "pull", - PullRequest: &api.PullRequest{ - Number: 103, - Repository: &api.PRRepository{ - NameWithOwner: "OWNER/REPO", - }, - }, - }, - { - ID: "id4", - Name: "s4", - State: "queued", - CreatedAt: sampleDate, - ResourceType: "pull", - PullRequest: &api.PullRequest{ - Number: 104, - Repository: &api.PRRepository{ - NameWithOwner: "OWNER/REPO", - }, - }, - }, - { - ID: "id5", - Name: "s5", - State: "canceled", - CreatedAt: sampleDate, - ResourceType: "pull", - PullRequest: &api.PullRequest{ - Number: 105, - Repository: &api.PRRepository{ - NameWithOwner: "OWNER/REPO", - }, - }, - }, - { - ID: "id6", - Name: "s6", - State: "mystery", - CreatedAt: sampleDate, - ResourceType: "pull", - PullRequest: &api.PullRequest{ - Number: 106, - Repository: &api.PRRepository{ - NameWithOwner: "OWNER/REPO", - }, - }, - }, - }, nil - } - }, - wantOut: heredoc.Doc(` - SESSION NAME PULL REQUEST REPO SESSION STATE CREATED - s1 #101 OWNER/REPO completed about 6 hours ago - s2 #102 OWNER/REPO failed about 6 hours ago - s3 #103 OWNER/REPO in_progress about 6 hours ago - s4 #104 OWNER/REPO queued about 6 hours ago - s5 #105 OWNER/REPO canceled about 6 hours ago - s6 #106 OWNER/REPO mystery about 6 hours ago - `), - }, - { - name: "repo resolution error does not surface", - tty: true, - baseRepoErr: errors.New("ambiguous repo"), - capiStubs: func(t *testing.T, m *capi.CapiClientMock) { - // We expect a viewer-scoped fetch request: - m.ListSessionsForViewerFunc = func(ctx context.Context, limit int) ([]*capi.Session, error) { - return nil, nil - } - }, - wantErr: cmdutil.NewNoResultsError("no agent tasks found"), - }, { name: "web mode", tty: true, @@ -490,15 +286,6 @@ func Test_listRun(t *testing.T) { wantStderr: "Opening https://github.com/copilot/agents in your browser.\n", wantBrowserURL: "https://github.com/copilot/agents", }, - { - name: "web mode with repo still uses global URL, even when --repo is set", - tty: true, - web: true, - baseRepo: ghrepo.New("OWNER", "REPO"), - wantOut: "", - wantStderr: "Opening https://github.com/copilot/agents in your browser.\n", - wantBrowserURL: "https://github.com/copilot/agents", - }, } for _, tt := range tests { @@ -528,11 +315,6 @@ func Test_listRun(t *testing.T) { return capiClientMock, nil }, } - if tt.baseRepo != nil || tt.baseRepoErr != nil { - baseRepo := tt.baseRepo - baseRepoErr := tt.baseRepoErr - opts.BaseRepo = func() (ghrepo.Interface, error) { return baseRepo, baseRepoErr } - } err := listRun(opts) if tt.wantErr != nil {