Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pkg/cli/copilot_billing_check.go
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ type orgCopilotBillingProbeResult struct {
// probeCopilotBillingForOrg probes the org's Copilot CLI billing status and
// returns derived UI hints for the auth method selection form.
func probeCopilotBillingForOrg(ctx context.Context, orgLogin string) orgCopilotBillingProbeResult {
client, err := api.NewRESTClient(api.ClientOptions{})
client, err := api.DefaultRESTClient()
if err != nil {
return orgCopilotBillingProbeResult{
InfoNote: copilotBillingInconclusiveNote,
Expand Down
2 changes: 1 addition & 1 deletion pkg/cli/update_cooldown.go
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ func defaultCoolDownDeps() coolDownDeps {
}

func getReleasePublishedAt(ctx context.Context, repo, tag string) (time.Time, error) {
client, err := api.NewRESTClient(api.ClientOptions{})
client, err := api.DefaultRESTClient()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Context passed in but not forwarded: getReleasePublishedAt accepts ctx context.Context yet passes no context to the REST call — client.Get(...) fires context.Background() internally, so the caller-supplied cancel/deadline is silently dropped.

💡 Suggested fix

Change client.Get(...) to client.DoWithContext(ctx, "GET", ..., nil, &release), matching what this PR already does for similar calls in repository_features_validation.go:

if err := client.DoWithContext(ctx, "GET", fmt.Sprintf("repos/%s/releases/tags/%s", repo, url.PathEscape(tag)), nil, &release); err != nil {

This is pre-existing, but the PR explicitly cites adding context/timeout support as a goal; the adjacent omission undermines that intent and could cause checkReleaseCoolDown to hang past any deadline set by the caller.

if err != nil {
return time.Time{}, fmt.Errorf("failed to create GitHub client: %w", err)
}
Expand Down
45 changes: 25 additions & 20 deletions pkg/workflow/repository_features_validation.go
Original file line number Diff line number Diff line change
Expand Up @@ -39,19 +39,23 @@
package workflow

import (
"encoding/json"
"context"
"fmt"
"os"
"strings"
"sync"
"time"

"github.com/cli/go-gh/v2"
"github.com/cli/go-gh/v2/pkg/api"
"github.com/cli/go-gh/v2/pkg/repository"
"github.com/github/gh-aw/pkg/console"
"github.com/github/gh-aw/pkg/syncutil"
)

// repositoryFeaturesTimeout is the per-request timeout for repository feature API calls
// (mirrors the copilot-billing probe timeout).
const repositoryFeaturesTimeout = 3 * time.Second

var repositoryFeaturesLog = newValidationLogger("repository_features")

// checkRepositoryHasDiscussionsQuery is a hardcoded static GraphQL query template used to check
Expand Down Expand Up @@ -268,28 +272,27 @@ func checkRepositoryHasDiscussionsUncached(repo string) (bool, error) {
}
owner, name := parts[0], parts[1]

// Execute GraphQL query using gh CLI.
// Use native GraphQL client — no gh binary dependency, native context/cancel support.
// checkRepositoryHasDiscussionsQuery is a package-level constant — not user-controlled.
type GraphQLResponse struct {
Data struct {
Repository struct {
HasDiscussionsEnabled bool `json:"hasDiscussionsEnabled"`
} `json:"repository"`
} `json:"data"`
client, err := api.DefaultGraphQLClient()
if err != nil {
return false, fmt.Errorf("failed to create GraphQL client: %w", err)
}

stdOut, _, err := gh.Exec("api", "graphql", "-f", "query="+checkRepositoryHasDiscussionsQuery,
"-f", "owner="+owner, "-f", "name="+name)
if err != nil {
return false, fmt.Errorf("failed to query discussions status: %w", err)
var response struct {
Repository struct {
HasDiscussionsEnabled bool `json:"hasDiscussionsEnabled"`
} `json:"repository"`
}

var response GraphQLResponse
if err := json.Unmarshal(stdOut.Bytes(), &response); err != nil {
return false, fmt.Errorf("failed to parse GraphQL response: %w", err)
ctx, cancel := context.WithTimeout(context.Background(), repositoryFeaturesTimeout)
defer cancel()

if err := client.DoWithContext(ctx, checkRepositoryHasDiscussionsQuery, map[string]any{"owner": owner, "name": name}, &response); err != nil {
return false, fmt.Errorf("failed to query discussions status: %w", err)
}

return response.Data.Repository.HasDiscussionsEnabled, nil
return response.Repository.HasDiscussionsEnabled, nil
}

// checkRepositoryHasIssues checks if a repository has issues enabled (with caching)
Expand All @@ -315,10 +318,12 @@ func checkRepositoryHasIssuesUncached(repo string) (bool, error) {
return false, fmt.Errorf("failed to create REST client: %w", err)
}

// Fetch repository data using REST client
ctx, cancel := context.WithTimeout(context.Background(), repositoryFeaturesTimeout)
defer cancel()

// Fetch repository data using REST client with timeout context
var response RepositoryResponse
err = client.Get("repos/"+repo, &response)
if err != nil {
if err := client.DoWithContext(ctx, "GET", "repos/"+repo, nil, &response); err != nil {
return false, fmt.Errorf("failed to query repository: %w", err)
}

Expand Down
Loading