From d84b8779933d1103f4407840f99e63442e559745 Mon Sep 17 00:00:00 2001 From: Antoine Toussaint Date: Sun, 23 Aug 2026 16:01:02 -0400 Subject: [PATCH 1/3] refactor(github): move GitOps PR and chore-issue flows to go-github (#458) Replace the `gh pr list/create/edit/view` and `gh issue create` shell-outs with go-github REST calls, and extract a shared `pkg/github` client/token helper reused by cmd/agents, pkg/gitops and cmd/status. Co-Authored-By: Claude Opus 4.8 --- cmd/agents/versions.go | 36 +------ cmd/agents/versions_test.go | 36 ------- cmd/status/release.go | 49 +++++++--- cmd/status/release_test.go | 73 ++++++++++++++ pkg/github/client.go | 45 +++++++++ pkg/github/client_test.go | 58 ++++++++++++ pkg/gitops/observe.go | 137 +++++++++++++++++++-------- pkg/gitops/observe_test.go | 34 +++---- pkg/gitops/publish.go | 75 ++++++++------- pkg/gitops/qualification_k3d_test.go | 49 +++++----- 10 files changed, 402 insertions(+), 190 deletions(-) create mode 100644 cmd/status/release_test.go create mode 100644 pkg/github/client.go create mode 100644 pkg/github/client_test.go diff --git a/cmd/agents/versions.go b/cmd/agents/versions.go index a116b6f4..6cf9649a 100644 --- a/cmd/agents/versions.go +++ b/cmd/agents/versions.go @@ -6,7 +6,6 @@ import ( "fmt" "net/http" "os" - "os/exec" "path/filepath" "slices" "sort" @@ -16,6 +15,7 @@ import ( "github.com/blang/semver" "github.com/codefly-dev/cli/cmd/common" "github.com/codefly-dev/cli/pkg/cli" + ghclient "github.com/codefly-dev/cli/pkg/github" "github.com/codefly-dev/core/resources" "github.com/google/go-github/v89/github" "github.com/spf13/cobra" @@ -416,7 +416,7 @@ func pinnedVersions(ctx context.Context, agent *resources.Agent) []string { } func fetchReleasesFromGitHub(ctx context.Context, agent *resources.Agent) ([]releaseInfo, error) { - client, err := newGitHubClient() + client, err := ghclient.NewClient() if err != nil { return nil, err } @@ -452,7 +452,7 @@ func fetchReleasesFromGitHub(ctx context.Context, agent *resources.Agent) ([]rel } func fetchTagsFromGitHub(ctx context.Context, agent *resources.Agent) ([]string, error) { - client, err := newGitHubClient() + client, err := ghclient.NewClient() if err != nil { return nil, err } @@ -481,36 +481,6 @@ func githubSource(agent *resources.Agent) (owner, repo string) { return strings.ReplaceAll(agent.Publisher, ".", "-"), "service-" + agent.Name } -// newGitHubClient returns a client authenticated with GITHUB_TOKEN/GH_TOKEN -// when either is set. Listing every version of every pinned agent multiplies -// requests fast, and the unauthenticated 60/hour limit turns this diagnostic -// flaky exactly when a workspace has many pins to check. -func newGitHubClient() (*github.Client, error) { - if token := githubToken(); token != "" { - return github.NewClient(github.WithAuthToken(token)) - } - return github.NewClient() -} - -// githubToken resolves a GitHub token from GITHUB_TOKEN/GH_TOKEN, falling back -// to the `gh` CLI's stored credential. Without the `gh` fallback, `agent list`/ -// `versions` runs unauthenticated (60 req/hour) and reports resolvable versions -// as "-" the moment a workspace has several pins to check — a confusing false -// negative on a machine that is in fact fully authenticated via `gh`. -func githubToken() string { - if t := strings.TrimSpace(os.Getenv("GITHUB_TOKEN")); t != "" { - return t - } - if t := strings.TrimSpace(os.Getenv("GH_TOKEN")); t != "" { - return t - } - out, err := exec.Command("gh", "auth", "token").Output() - if err != nil { - return "" - } - return strings.TrimSpace(string(out)) -} - func localCacheVersions(ctx context.Context, agent *resources.Agent) []string { dir := filepath.Join(resources.AgentBase(ctx), "agents", agentSubdir(agent), agent.Publisher) entries, err := os.ReadDir(dir) diff --git a/cmd/agents/versions_test.go b/cmd/agents/versions_test.go index 6b0e47ba..0251cf69 100644 --- a/cmd/agents/versions_test.go +++ b/cmd/agents/versions_test.go @@ -303,42 +303,6 @@ func TestLocalCacheVersionsScansAgentDir(t *testing.T) { } } -func TestNewGitHubClientAddsAuthorization(t *testing.T) { - t.Setenv("GITHUB_TOKEN", "secret") - var got string - server := httptest.NewServer(http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) { - got = r.Header.Get("Authorization") - })) - defer server.Close() - - client, err := newGitHubClient() - if err != nil { - t.Fatalf("newGitHubClient: %v", err) - } - resp, err := client.Client().Get(server.URL) - if err != nil { - t.Fatal(err) - } - resp.Body.Close() - if got != "Bearer secret" { - t.Fatalf("Authorization = %q, want %q", got, "Bearer secret") - } -} - -func TestNewGitHubClientUnauthenticated(t *testing.T) { - t.Setenv("GITHUB_TOKEN", "") - t.Setenv("GH_TOKEN", "") - t.Setenv("PATH", "") // no `gh` on PATH: force the tokenless path - - client, err := newGitHubClient() - if err != nil { - t.Fatalf("newGitHubClient: %v", err) - } - if client == nil { - t.Fatal("newGitHubClient returned a nil client") - } -} - func TestSummarizeWorkspaceAgentsCachesAndFlagsResolvability(t *testing.T) { restoreReleases, restoreTags, restoreOCI := fetchReleases, fetchTags, fetchOCITags defer func() { fetchReleases, fetchTags, fetchOCITags = restoreReleases, restoreTags, restoreOCI }() diff --git a/cmd/status/release.go b/cmd/status/release.go index 620418ac..66951414 100644 --- a/cmd/status/release.go +++ b/cmd/status/release.go @@ -9,7 +9,9 @@ import ( "sort" "strings" + ghclient "github.com/codefly-dev/cli/pkg/github" "github.com/fatih/color" + "github.com/google/go-github/v89/github" "github.com/spf13/cobra" ) @@ -261,18 +263,43 @@ func createAgentIssue(baseDir string, status AgentStatus) error { status.CoreVer, status.LatestCore, status.Delta, agentPath) - // Use gh to create issue - cmd := exec.CommandContext(context.Background(), - "gh", "issue", "create", - "--title", title, - "--body", body, - "--label", "chore", - "--label", "dependencies") - cmd.Dir = agentPath - - if err := cmd.Run(); err != nil { + owner, repo, err := agentRepository(agentPath) + if err != nil { return err } + client, err := ghclient.NewClient() + if err != nil { + return err + } + _, _, err = client.Issues.Create(context.Background(), owner, repo, &github.IssueRequest{ + Title: github.Ptr(title), + Body: github.Ptr(body), + Labels: &[]string{"chore", "dependencies"}, + }) + return err +} + +func agentRepository(agentPath string) (string, string, error) { + out, err := exec.Command("git", "-C", agentPath, "remote", "get-url", "origin").Output() + if err != nil { + return "", "", fmt.Errorf("resolve %s origin remote: %w", agentPath, err) + } + return parseGitHubRemote(strings.TrimSpace(string(out))) +} - return nil +func parseGitHubRemote(remote string) (string, string, error) { + trimmed := strings.TrimSuffix(remote, ".git") + switch { + case strings.HasPrefix(trimmed, "git@github.com:"): + trimmed = strings.TrimPrefix(trimmed, "git@github.com:") + case strings.HasPrefix(trimmed, "https://github.com/"): + trimmed = strings.TrimPrefix(trimmed, "https://github.com/") + default: + return "", "", fmt.Errorf("unrecognized GitHub remote %q", remote) + } + owner, repo, ok := strings.Cut(trimmed, "/") + if !ok || owner == "" || repo == "" { + return "", "", fmt.Errorf("unrecognized GitHub remote %q", remote) + } + return owner, repo, nil } diff --git a/cmd/status/release_test.go b/cmd/status/release_test.go new file mode 100644 index 00000000..e8230fa0 --- /dev/null +++ b/cmd/status/release_test.go @@ -0,0 +1,73 @@ +package status + +import ( + "encoding/json" + "io" + "net/http" + "net/http/httptest" + "os/exec" + "path/filepath" + "testing" + + "github.com/google/go-github/v89/github" +) + +func TestParseGitHubRemote(t *testing.T) { + cases := []struct { + remote string + owner string + repo string + ok bool + }{ + {"git@github.com:codefly-dev/service-redis.git", "codefly-dev", "service-redis", true}, + {"https://github.com/codefly-dev/service-redis.git", "codefly-dev", "service-redis", true}, + {"https://github.com/codefly-dev/service-redis", "codefly-dev", "service-redis", true}, + {"git@gitlab.com:codefly-dev/service-redis.git", "", "", false}, + {"https://github.com/codefly-dev", "", "", false}, + } + for _, tc := range cases { + owner, repo, err := parseGitHubRemote(tc.remote) + if tc.ok != (err == nil) { + t.Fatalf("%s: err = %v, want ok=%v", tc.remote, err, tc.ok) + } + if tc.ok && (owner != tc.owner || repo != tc.repo) { + t.Fatalf("%s: got %s/%s, want %s/%s", tc.remote, owner, repo, tc.owner, tc.repo) + } + } +} + +func TestCreateAgentIssueCreatesIssueThroughAPI(t *testing.T) { + baseDir := t.TempDir() + agentPath := filepath.Join(baseDir, "service-redis") + if err := exec.Command("git", "init", "-q", agentPath).Run(); err != nil { + t.Fatal(err) + } + if err := exec.Command("git", "-C", agentPath, "remote", "add", "origin", + "git@github.com:codefly-dev/service-redis.git").Run(); err != nil { + t.Fatal(err) + } + + var gotPath string + var payload github.IssueRequest + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + gotPath = r.URL.Path + body, _ := io.ReadAll(r.Body) + _ = json.Unmarshal(body, &payload) + w.WriteHeader(http.StatusCreated) + w.Write([]byte(`{"number":7,"html_url":"https://github.com/codefly-dev/service-redis/issues/7"}`)) + })) + defer server.Close() + t.Setenv("GITHUB_TOKEN", "test-token") + t.Setenv("GITHUB_API_URL", server.URL) + + status := AgentStatus{Name: "service-redis", CoreVer: "0.3.0", LatestCore: "0.3.5", Delta: 5} + if err := createAgentIssue(baseDir, status); err != nil { + t.Fatalf("createAgentIssue: %v", err) + } + if gotPath != "/api/v3/repos/codefly-dev/service-redis/issues" { + t.Fatalf("request path = %q", gotPath) + } + if payload.Labels == nil || len(*payload.Labels) != 2 { + t.Fatalf("labels = %v, want [chore dependencies]", payload.Labels) + } +} diff --git a/pkg/github/client.go b/pkg/github/client.go new file mode 100644 index 00000000..46a69ffd --- /dev/null +++ b/pkg/github/client.go @@ -0,0 +1,45 @@ +// Package github centralizes construction of an authenticated go-github client +// so every caller shares one token-resolution and endpoint policy instead of +// re-deriving the plumbing. +package github + +import ( + "os" + "os/exec" + "strings" + + gogithub "github.com/google/go-github/v89/github" +) + +// NewClient returns a go-github client authenticated with the resolved token +// when one is available and unauthenticated otherwise. When GITHUB_API_URL is +// set — as it is inside GitHub Actions and against GitHub Enterprise — the +// client is pointed at that endpoint. +func NewClient() (*gogithub.Client, error) { + var options []gogithub.ClientOptionsFunc + if token := Token(); token != "" { + options = append(options, gogithub.WithAuthToken(token)) + } + if endpoint := strings.TrimSpace(os.Getenv("GITHUB_API_URL")); endpoint != "" { + options = append(options, gogithub.WithEnterpriseURLs(endpoint, endpoint)) + } + return gogithub.NewClient(options...) +} + +// Token resolves a GitHub token from GITHUB_TOKEN/GH_TOKEN, falling back to the +// gh CLI's stored credential. Without the gh fallback, developer/CI callers run +// unauthenticated (60 requests/hour) on machines that are in fact fully +// authenticated via gh. +func Token() string { + if t := strings.TrimSpace(os.Getenv("GITHUB_TOKEN")); t != "" { + return t + } + if t := strings.TrimSpace(os.Getenv("GH_TOKEN")); t != "" { + return t + } + out, err := exec.Command("gh", "auth", "token").Output() + if err != nil { + return "" + } + return strings.TrimSpace(string(out)) +} diff --git a/pkg/github/client_test.go b/pkg/github/client_test.go new file mode 100644 index 00000000..2d91c69a --- /dev/null +++ b/pkg/github/client_test.go @@ -0,0 +1,58 @@ +package github + +import ( + "net/http" + "net/http/httptest" + "testing" +) + +func TestNewClientAddsAuthorization(t *testing.T) { + t.Setenv("GITHUB_TOKEN", "secret") + var got string + server := httptest.NewServer(http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) { + got = r.Header.Get("Authorization") + })) + defer server.Close() + + client, err := NewClient() + if err != nil { + t.Fatalf("NewClient: %v", err) + } + resp, err := client.Client().Get(server.URL) + if err != nil { + t.Fatal(err) + } + resp.Body.Close() + if got != "Bearer secret" { + t.Fatalf("Authorization = %q, want %q", got, "Bearer secret") + } +} + +func TestNewClientUnauthenticated(t *testing.T) { + t.Setenv("GITHUB_TOKEN", "") + t.Setenv("GH_TOKEN", "") + t.Setenv("PATH", "") // no `gh` on PATH: force the tokenless path + + client, err := NewClient() + if err != nil { + t.Fatalf("NewClient: %v", err) + } + if client == nil { + t.Fatal("NewClient returned a nil client") + } +} + +func TestNewClientHonorsAPIEndpoint(t *testing.T) { + t.Setenv("GITHUB_TOKEN", "") + t.Setenv("GH_TOKEN", "") + t.Setenv("PATH", "") + t.Setenv("GITHUB_API_URL", "https://ghe.example.com/api/v3") + + client, err := NewClient() + if err != nil { + t.Fatalf("NewClient: %v", err) + } + if got := client.BaseURL(); got != "https://ghe.example.com/api/v3/" { + t.Fatalf("BaseURL = %q, want trailing-slash normalized endpoint", got) + } +} diff --git a/pkg/gitops/observe.go b/pkg/gitops/observe.go index 8852b635..9677eda1 100644 --- a/pkg/gitops/observe.go +++ b/pkg/gitops/observe.go @@ -12,8 +12,12 @@ import ( "reflect" "regexp" "sort" + "strconv" "strings" "time" + + ghclient "github.com/codefly-dev/cli/pkg/github" + "github.com/google/go-github/v89/github" ) var ( @@ -723,43 +727,43 @@ func observeReview(ctx context.Context, pullRequest, publishedCommit, repository if len(segments) != 4 || segments[0]+"/"+strings.TrimSuffix(segments[1], ".git") != repositorySlug { return ReviewEvidence{}, fmt.Errorf("promotion pull request repository differs from published repository") } - output, err := command(ctx, "", "gh", "pr", "view", pullRequest, - "--json", "url,state,reviewDecision,reviews,mergeCommit,commits") + owner, repo, err := splitRepositorySlug(repositorySlug) + if err != nil { + return ReviewEvidence{}, err + } + number, err := strconv.Atoi(segments[3]) + if err != nil { + return ReviewEvidence{}, fmt.Errorf("parse promotion pull request number: %w", err) + } + client, err := ghclient.NewClient() + if err != nil { + return ReviewEvidence{}, err + } + pullRequestResource, _, err := client.PullRequests.Get(ctx, owner, repo, number) + if err != nil { + return ReviewEvidence{}, fmt.Errorf("observe promotion review: %w", err) + } + if pullRequestResource.GetHTMLURL() != pullRequest { + return ReviewEvidence{}, fmt.Errorf("GitHub returned promotion pull request %s, expected %s", pullRequestResource.GetHTMLURL(), pullRequest) + } + if !pullRequestResource.GetMerged() { + return ReviewEvidence{}, fmt.Errorf("promotion pull request is %s, expected MERGED", strings.ToUpper(pullRequestResource.GetState())) + } + reviews, err := listPullRequestReviews(ctx, client, owner, repo, number) if err != nil { return ReviewEvidence{}, fmt.Errorf("observe promotion review: %w", err) } - var response struct { - URL string `json:"url"` - State string `json:"state"` - ReviewDecision string `json:"reviewDecision"` - Reviews []struct { - State string `json:"state"` - Author struct { - Login string `json:"login"` - } `json:"author"` - } `json:"reviews"` - MergeCommit struct { - OID string `json:"oid"` - } `json:"mergeCommit"` - Commits []struct { - OID string `json:"oid"` - } `json:"commits"` - } - if err := json.Unmarshal([]byte(output), &response); err != nil { - return ReviewEvidence{}, fmt.Errorf("decode promotion review: %w", err) - } - if response.URL != pullRequest { - return ReviewEvidence{}, fmt.Errorf("GitHub returned promotion pull request %s, expected %s", response.URL, pullRequest) - } - if response.State != "MERGED" { - return ReviewEvidence{}, fmt.Errorf("promotion pull request is %s, expected MERGED", response.State) - } - if response.ReviewDecision != approvedReviewDecision { - return ReviewEvidence{}, fmt.Errorf("promotion pull request review decision is %s, expected APPROVED", response.ReviewDecision) + decision := deriveReviewDecision(reviews) + if decision != approvedReviewDecision { + return ReviewEvidence{}, fmt.Errorf("promotion pull request review decision is %s, expected APPROVED", decision) + } + commits, err := listPullRequestCommits(ctx, client, owner, repo, number) + if err != nil { + return ReviewEvidence{}, fmt.Errorf("observe promotion review: %w", err) } published := false - for _, commit := range response.Commits { - if commit.OID == publishedCommit { + for _, commit := range commits { + if commit.GetSHA() == publishedCommit { published = true break } @@ -768,12 +772,12 @@ func observeReview(ctx context.Context, pullRequest, publishedCommit, repository return ReviewEvidence{}, fmt.Errorf("promotion pull request does not contain signed commit %s", publishedCommit) } evidence := ReviewEvidence{ - URL: response.URL, State: response.State, ReviewDecision: response.ReviewDecision, - MergeCommit: response.MergeCommit.OID, + URL: pullRequestResource.GetHTMLURL(), State: "MERGED", ReviewDecision: decision, + MergeCommit: pullRequestResource.GetMergeCommitSHA(), } - for _, review := range response.Reviews { - if review.State == approvedReviewDecision && review.Author.Login != "" { - evidence.Reviewers = append(evidence.Reviewers, review.Author.Login) + for _, review := range reviews { + if review.GetState() == approvedReviewDecision && review.GetUser().GetLogin() != "" { + evidence.Reviewers = append(evidence.Reviewers, review.GetUser().GetLogin()) } } sort.Strings(evidence.Reviewers) @@ -782,3 +786,62 @@ func observeReview(ctx context.Context, pullRequest, publishedCommit, repository } return evidence, nil } + +// deriveReviewDecision reproduces GitHub's pull-request review decision from the +// REST review list — a field only the GraphQL API exposes directly. Only each +// author's latest APPROVED/CHANGES_REQUESTED/DISMISSED review counts; a single +// outstanding change request blocks approval, and at least one standing approval +// is required. +func deriveReviewDecision(reviews []*github.PullRequestReview) string { + latest := make(map[string]string) + for _, review := range reviews { + switch review.GetState() { + case "APPROVED", "CHANGES_REQUESTED", "DISMISSED": + latest[review.GetUser().GetLogin()] = review.GetState() + } + } + decision := "REVIEW_REQUIRED" + for _, state := range latest { + if state == "CHANGES_REQUESTED" { + return "CHANGES_REQUESTED" + } + if state == approvedReviewDecision { + decision = approvedReviewDecision + } + } + return decision +} + +func listPullRequestReviews(ctx context.Context, client *github.Client, owner, repo string, number int) ([]*github.PullRequestReview, error) { + var all []*github.PullRequestReview + opts := &github.ListOptions{PerPage: 100} + for { + reviews, resp, err := client.PullRequests.ListReviews(ctx, owner, repo, number, opts) + if err != nil { + return nil, err + } + all = append(all, reviews...) + if resp.NextPage == 0 { + break + } + opts.Page = resp.NextPage + } + return all, nil +} + +func listPullRequestCommits(ctx context.Context, client *github.Client, owner, repo string, number int) ([]*github.RepositoryCommit, error) { + var all []*github.RepositoryCommit + opts := &github.ListOptions{PerPage: 100} + for { + commits, resp, err := client.PullRequests.ListCommits(ctx, owner, repo, number, opts) + if err != nil { + return nil, err + } + all = append(all, commits...) + if resp.NextPage == 0 { + break + } + opts.Page = resp.NextPage + } + return all, nil +} diff --git a/pkg/gitops/observe_test.go b/pkg/gitops/observe_test.go index c302044e..9e40bf89 100644 --- a/pkg/gitops/observe_test.go +++ b/pkg/gitops/observe_test.go @@ -3,6 +3,8 @@ package gitops import ( "context" "fmt" + "net/http" + "net/http/httptest" "os" "path/filepath" "strings" @@ -258,23 +260,21 @@ func TestObserveRejectsUnverifiedLocalReviewReference(t *testing.T) { } func TestObserveReviewProvesApprovalMergeAndPublishedCommit(t *testing.T) { - bin := t.TempDir() - script := filepath.Join(bin, "gh") - content := `#!/bin/sh -printf '%s\n' "$CODEFLY_TEST_GH_RESPONSE" -` - if err := os.WriteFile(script, []byte(content), 0o755); err != nil { - t.Fatal(err) - } - t.Setenv("PATH", bin+string(os.PathListSeparator)+os.Getenv("PATH")) - t.Setenv("CODEFLY_TEST_GH_RESPONSE", `{ - "url":"https://github.com/codefly-dev/manifests/pull/42", - "state":"MERGED", - "reviewDecision":"APPROVED", - "reviews":[{"state":"APPROVED","author":{"login":"reviewer"}}], - "mergeCommit":{"oid":"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"}, - "commits":[{"oid":"cccccccccccccccccccccccccccccccccccccccc"}] -}`) + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch { + case strings.HasSuffix(r.URL.Path, "/pulls/42/reviews"): + fmt.Fprint(w, `[{"state":"APPROVED","user":{"login":"reviewer"}}]`) + case strings.HasSuffix(r.URL.Path, "/pulls/42/commits"): + fmt.Fprintf(w, `[{"sha":%q}]`, signedCommit) + case strings.HasSuffix(r.URL.Path, "/pulls/42"): + fmt.Fprintf(w, `{"number":42,"html_url":"https://github.com/codefly-dev/manifests/pull/42","state":"closed","merged":true,"merge_commit_sha":%q}`, observedRevision) + default: + http.NotFound(w, r) + } + })) + defer server.Close() + t.Setenv("GITHUB_TOKEN", "test-token") + t.Setenv("GITHUB_API_URL", server.URL) review, err := observeReview(context.Background(), "https://github.com/codefly-dev/manifests/pull/42", signedCommit, "https://github.com/codefly-dev/manifests.git", false) diff --git a/pkg/gitops/publish.go b/pkg/gitops/publish.go index a6643246..c8d74b7b 100644 --- a/pkg/gitops/publish.go +++ b/pkg/gitops/publish.go @@ -16,9 +16,11 @@ import ( "strconv" "strings" + ghclient "github.com/codefly-dev/cli/pkg/github" "github.com/codefly-dev/cli/pkg/internal/mutationauthority" "github.com/codefly-dev/cli/pkg/orchestration" "github.com/codefly-dev/core/resources" + "github.com/google/go-github/v89/github" "gopkg.in/yaml.v3" ) @@ -1251,69 +1253,74 @@ func openOrUpdatePullRequest(ctx context.Context, prepared *preparedRepository, commit, ) } - output, err := command(ctx, "", "gh", "pr", "list", - "--repo", prepared.plan.RepositorySlug, "--head", prepared.plan.PromotionBranch, - "--base", prepared.plan.BaseBranch, "--state", "open", "--json", "number,url,headRefOid") + owner, repo, err := splitRepositorySlug(prepared.plan.RepositorySlug) if err != nil { - return "", 0, fmt.Errorf("inspect promotion pull request: %w", err) + return "", 0, err } - var existing []struct { - Number int `json:"number"` - URL string `json:"url"` - HeadRefOID string `json:"headRefOid"` + client, err := ghclient.NewClient() + if err != nil { + return "", 0, err } - if err := json.Unmarshal([]byte(output), &existing); err != nil { - return "", 0, fmt.Errorf("decode promotion pull request: %w", err) + existing, _, err := client.PullRequests.List(ctx, owner, repo, &github.PullRequestListOptions{ + State: "open", + Head: owner + ":" + prepared.plan.PromotionBranch, + Base: prepared.plan.BaseBranch, + }) + if err != nil { + return "", 0, fmt.Errorf("inspect promotion pull request: %w", err) } if len(existing) > 1 { return "", 0, fmt.Errorf("multiple open promotion pull requests target %s", prepared.plan.PromotionBranch) } if len(existing) == 1 { pr := existing[0] - if pr.HeadRefOID != commit { - return "", 0, fmt.Errorf("pull request head is %s, expected %s", pr.HeadRefOID, commit) + if pr.GetHead().GetSHA() != commit { + return "", 0, fmt.Errorf("pull request head is %s, expected %s", pr.GetHead().GetSHA(), commit) } - if _, err := command(ctx, "", "gh", "pr", "edit", strconv.Itoa(pr.Number), - "--repo", prepared.plan.RepositorySlug, "--title", title, "--body", body); err != nil { + if _, _, err := client.PullRequests.Edit(ctx, owner, repo, pr.GetNumber(), &github.PullRequest{ + Title: github.Ptr(title), + Body: github.Ptr(body), + }); err != nil { return "", 0, fmt.Errorf("update promotion pull request: %w", err) } - return pr.URL, pr.Number, nil + return pr.GetHTMLURL(), pr.GetNumber(), nil } - url, err := command(ctx, "", "gh", "pr", "create", "--repo", prepared.plan.RepositorySlug, - "--base", prepared.plan.BaseBranch, "--head", prepared.plan.PromotionBranch, - "--title", title, "--body", body) + created, _, err := client.PullRequests.Create(ctx, owner, repo, &github.NewPullRequest{ + Title: github.Ptr(title), + Head: github.Ptr(prepared.plan.PromotionBranch), + Base: github.Ptr(prepared.plan.BaseBranch), + Body: github.Ptr(body), + }) if err != nil { return "", 0, fmt.Errorf("open promotion pull request: %w", err) } - return verifyPullRequest(ctx, prepared.plan.RepositorySlug, strings.TrimSpace(url), prepared.plan.BaseBranch, commit) + return verifyPullRequest(ctx, client, owner, repo, created.GetNumber(), prepared.plan.BaseBranch, commit) +} + +func splitRepositorySlug(slug string) (string, string, error) { + owner, repo, ok := strings.Cut(slug, "/") + if !ok || owner == "" || repo == "" { + return "", "", fmt.Errorf("invalid repository %q", slug) + } + return owner, repo, nil } func localReviewRef(promotionBranch, commit string) string { return "refs/codefly/reviews/" + strings.ReplaceAll(promotionBranch, "/", "-") + "/" + commit } -func verifyPullRequest(ctx context.Context, repository, pullRequest, baseBranch, commit string) (string, int, error) { - output, err := command(ctx, "", "gh", "pr", "view", pullRequest, "--repo", repository, - "--json", "number,url,headRefOid,baseRefName") +func verifyPullRequest(ctx context.Context, client *github.Client, owner, repo string, number int, baseBranch, commit string) (string, int, error) { + pr, _, err := client.PullRequests.Get(ctx, owner, repo, number) if err != nil { return "", 0, fmt.Errorf("verify promotion pull request: %w", err) } - var response struct { - Number int `json:"number"` - URL string `json:"url"` - HeadRefOID string `json:"headRefOid"` - BaseRefName string `json:"baseRefName"` - } - if err := json.Unmarshal([]byte(output), &response); err != nil { - return "", 0, fmt.Errorf("decode verified promotion pull request: %w", err) - } - if response.HeadRefOID != commit || response.BaseRefName != baseBranch { + if pr.GetHead().GetSHA() != commit || pr.GetBase().GetRef() != baseBranch { return "", 0, fmt.Errorf( "promotion pull request targets %s at %s, expected %s at %s", - response.BaseRefName, response.HeadRefOID, baseBranch, commit, + pr.GetBase().GetRef(), pr.GetHead().GetSHA(), baseBranch, commit, ) } - return response.URL, response.Number, nil + return pr.GetHTMLURL(), pr.GetNumber(), nil } func resolveGitops(workspace *resources.Workspace, environment string, local bool) (*repositoryConfig, string, string, string, error) { diff --git a/pkg/gitops/qualification_k3d_test.go b/pkg/gitops/qualification_k3d_test.go index 06d90c4d..609dd287 100644 --- a/pkg/gitops/qualification_k3d_test.go +++ b/pkg/gitops/qualification_k3d_test.go @@ -3,6 +3,8 @@ package gitops import ( "context" "fmt" + "net/http" + "net/http/httptest" "os" "os/exec" "path/filepath" @@ -75,29 +77,32 @@ gitops: if err := os.WriteFile(kubectl, []byte("#!/bin/sh\ntouch \"$CODEFLY_TEST_KUBECTL_CALLED\"\nexit 97\n"), 0o755); err != nil { t.Fatal(err) } - gh := filepath.Join(bin, "gh") - ghScript := `#!/bin/sh -set -eu -if [ "$1 $2" = "pr list" ]; then - printf '%s\n' '[]' - exit 0 -fi -if [ "$1 $2" = "pr create" ]; then - printf '%s\n' 'https://github.com/codefly-test/manifests/pull/1' - exit 0 -fi -if [ "$1 $2" = "pr view" ]; then - revision="$(git --git-dir "$CODEFLY_TEST_REMOTE" rev-parse refs/heads/codefly/promote-payments-aws)" - printf '{"number":1,"url":"https://github.com/codefly-test/manifests/pull/1","headRefOid":"%s","baseRefName":"main"}\n' "$revision" - exit 0 -fi -exit 2 -` - if err := os.WriteFile(gh, []byte(ghScript), 0o755); err != nil { - t.Fatal(err) - } + branchRevision := func() string { + out, err := exec.Command("git", "--git-dir", remote, "rev-parse", "refs/heads/codefly/promote-payments-aws").Output() + if err != nil { + return "" + } + return strings.TrimSpace(string(out)) + } + pullRequest := func() string { + return fmt.Sprintf(`{"number":1,"html_url":"https://github.com/codefly-test/manifests/pull/1","head":{"sha":%q},"base":{"ref":"main"}}`, branchRevision()) + } + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch { + case r.Method == http.MethodGet && strings.HasSuffix(r.URL.Path, "/pulls"): + fmt.Fprint(w, `[]`) + case r.Method == http.MethodPost && strings.HasSuffix(r.URL.Path, "/pulls"): + fmt.Fprint(w, pullRequest()) + case strings.HasSuffix(r.URL.Path, "/pulls/1"): + fmt.Fprint(w, pullRequest()) + default: + http.NotFound(w, r) + } + })) + defer server.Close() t.Setenv("CODEFLY_TEST_KUBECTL_CALLED", kubectlCalled) - t.Setenv("CODEFLY_TEST_REMOTE", remote) + t.Setenv("GITHUB_TOKEN", "test-token") + t.Setenv("GITHUB_API_URL", server.URL) t.Setenv("PATH", bin+string(os.PathListSeparator)+os.Getenv("PATH")) request := PublishRequest{ From 2811b366d58920bb4217a1c8a7954ba449ced756 Mon Sep 17 00:00:00 2001 From: Antoine Toussaint Date: Sun, 23 Aug 2026 17:39:52 -0400 Subject: [PATCH 2/3] fix(gitops): keep observe review gate on gh; cover PR edit path (#458) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The review-decision gate in observeReview was migrated to a REST-derived approval check. GitHub's reviewDecision is a GraphQL-computed field that folds in branch-protection required-approval counts and CODEOWNERS; a REST ">=1 approval, no change request" derivation cannot see those. A promotion PR with a single non-required approval, merged via admin/maintainer bypass, would derive APPROVED and pass the deploy-authorization gate that the old `gh pr view --json reviewDecision` correctly rejected. Revert observe.go to gh — reviewDecision has no faithful REST equivalent. Add tests for the PR update path in openOrUpdatePullRequest (existing open promotion is edited; head-commit drift is rejected), which the go-github migration previously left uncovered. Co-Authored-By: Claude Opus 4.8 --- pkg/gitops/observe.go | 137 ++++++++++--------------------------- pkg/gitops/observe_test.go | 34 ++++----- pkg/gitops/publish_test.go | 63 +++++++++++++++++ 3 files changed, 117 insertions(+), 117 deletions(-) diff --git a/pkg/gitops/observe.go b/pkg/gitops/observe.go index 9677eda1..8852b635 100644 --- a/pkg/gitops/observe.go +++ b/pkg/gitops/observe.go @@ -12,12 +12,8 @@ import ( "reflect" "regexp" "sort" - "strconv" "strings" "time" - - ghclient "github.com/codefly-dev/cli/pkg/github" - "github.com/google/go-github/v89/github" ) var ( @@ -727,43 +723,43 @@ func observeReview(ctx context.Context, pullRequest, publishedCommit, repository if len(segments) != 4 || segments[0]+"/"+strings.TrimSuffix(segments[1], ".git") != repositorySlug { return ReviewEvidence{}, fmt.Errorf("promotion pull request repository differs from published repository") } - owner, repo, err := splitRepositorySlug(repositorySlug) - if err != nil { - return ReviewEvidence{}, err - } - number, err := strconv.Atoi(segments[3]) - if err != nil { - return ReviewEvidence{}, fmt.Errorf("parse promotion pull request number: %w", err) - } - client, err := ghclient.NewClient() - if err != nil { - return ReviewEvidence{}, err - } - pullRequestResource, _, err := client.PullRequests.Get(ctx, owner, repo, number) - if err != nil { - return ReviewEvidence{}, fmt.Errorf("observe promotion review: %w", err) - } - if pullRequestResource.GetHTMLURL() != pullRequest { - return ReviewEvidence{}, fmt.Errorf("GitHub returned promotion pull request %s, expected %s", pullRequestResource.GetHTMLURL(), pullRequest) - } - if !pullRequestResource.GetMerged() { - return ReviewEvidence{}, fmt.Errorf("promotion pull request is %s, expected MERGED", strings.ToUpper(pullRequestResource.GetState())) - } - reviews, err := listPullRequestReviews(ctx, client, owner, repo, number) + output, err := command(ctx, "", "gh", "pr", "view", pullRequest, + "--json", "url,state,reviewDecision,reviews,mergeCommit,commits") if err != nil { return ReviewEvidence{}, fmt.Errorf("observe promotion review: %w", err) } - decision := deriveReviewDecision(reviews) - if decision != approvedReviewDecision { - return ReviewEvidence{}, fmt.Errorf("promotion pull request review decision is %s, expected APPROVED", decision) - } - commits, err := listPullRequestCommits(ctx, client, owner, repo, number) - if err != nil { - return ReviewEvidence{}, fmt.Errorf("observe promotion review: %w", err) + var response struct { + URL string `json:"url"` + State string `json:"state"` + ReviewDecision string `json:"reviewDecision"` + Reviews []struct { + State string `json:"state"` + Author struct { + Login string `json:"login"` + } `json:"author"` + } `json:"reviews"` + MergeCommit struct { + OID string `json:"oid"` + } `json:"mergeCommit"` + Commits []struct { + OID string `json:"oid"` + } `json:"commits"` + } + if err := json.Unmarshal([]byte(output), &response); err != nil { + return ReviewEvidence{}, fmt.Errorf("decode promotion review: %w", err) + } + if response.URL != pullRequest { + return ReviewEvidence{}, fmt.Errorf("GitHub returned promotion pull request %s, expected %s", response.URL, pullRequest) + } + if response.State != "MERGED" { + return ReviewEvidence{}, fmt.Errorf("promotion pull request is %s, expected MERGED", response.State) + } + if response.ReviewDecision != approvedReviewDecision { + return ReviewEvidence{}, fmt.Errorf("promotion pull request review decision is %s, expected APPROVED", response.ReviewDecision) } published := false - for _, commit := range commits { - if commit.GetSHA() == publishedCommit { + for _, commit := range response.Commits { + if commit.OID == publishedCommit { published = true break } @@ -772,12 +768,12 @@ func observeReview(ctx context.Context, pullRequest, publishedCommit, repository return ReviewEvidence{}, fmt.Errorf("promotion pull request does not contain signed commit %s", publishedCommit) } evidence := ReviewEvidence{ - URL: pullRequestResource.GetHTMLURL(), State: "MERGED", ReviewDecision: decision, - MergeCommit: pullRequestResource.GetMergeCommitSHA(), + URL: response.URL, State: response.State, ReviewDecision: response.ReviewDecision, + MergeCommit: response.MergeCommit.OID, } - for _, review := range reviews { - if review.GetState() == approvedReviewDecision && review.GetUser().GetLogin() != "" { - evidence.Reviewers = append(evidence.Reviewers, review.GetUser().GetLogin()) + for _, review := range response.Reviews { + if review.State == approvedReviewDecision && review.Author.Login != "" { + evidence.Reviewers = append(evidence.Reviewers, review.Author.Login) } } sort.Strings(evidence.Reviewers) @@ -786,62 +782,3 @@ func observeReview(ctx context.Context, pullRequest, publishedCommit, repository } return evidence, nil } - -// deriveReviewDecision reproduces GitHub's pull-request review decision from the -// REST review list — a field only the GraphQL API exposes directly. Only each -// author's latest APPROVED/CHANGES_REQUESTED/DISMISSED review counts; a single -// outstanding change request blocks approval, and at least one standing approval -// is required. -func deriveReviewDecision(reviews []*github.PullRequestReview) string { - latest := make(map[string]string) - for _, review := range reviews { - switch review.GetState() { - case "APPROVED", "CHANGES_REQUESTED", "DISMISSED": - latest[review.GetUser().GetLogin()] = review.GetState() - } - } - decision := "REVIEW_REQUIRED" - for _, state := range latest { - if state == "CHANGES_REQUESTED" { - return "CHANGES_REQUESTED" - } - if state == approvedReviewDecision { - decision = approvedReviewDecision - } - } - return decision -} - -func listPullRequestReviews(ctx context.Context, client *github.Client, owner, repo string, number int) ([]*github.PullRequestReview, error) { - var all []*github.PullRequestReview - opts := &github.ListOptions{PerPage: 100} - for { - reviews, resp, err := client.PullRequests.ListReviews(ctx, owner, repo, number, opts) - if err != nil { - return nil, err - } - all = append(all, reviews...) - if resp.NextPage == 0 { - break - } - opts.Page = resp.NextPage - } - return all, nil -} - -func listPullRequestCommits(ctx context.Context, client *github.Client, owner, repo string, number int) ([]*github.RepositoryCommit, error) { - var all []*github.RepositoryCommit - opts := &github.ListOptions{PerPage: 100} - for { - commits, resp, err := client.PullRequests.ListCommits(ctx, owner, repo, number, opts) - if err != nil { - return nil, err - } - all = append(all, commits...) - if resp.NextPage == 0 { - break - } - opts.Page = resp.NextPage - } - return all, nil -} diff --git a/pkg/gitops/observe_test.go b/pkg/gitops/observe_test.go index 9e40bf89..c302044e 100644 --- a/pkg/gitops/observe_test.go +++ b/pkg/gitops/observe_test.go @@ -3,8 +3,6 @@ package gitops import ( "context" "fmt" - "net/http" - "net/http/httptest" "os" "path/filepath" "strings" @@ -260,21 +258,23 @@ func TestObserveRejectsUnverifiedLocalReviewReference(t *testing.T) { } func TestObserveReviewProvesApprovalMergeAndPublishedCommit(t *testing.T) { - server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - switch { - case strings.HasSuffix(r.URL.Path, "/pulls/42/reviews"): - fmt.Fprint(w, `[{"state":"APPROVED","user":{"login":"reviewer"}}]`) - case strings.HasSuffix(r.URL.Path, "/pulls/42/commits"): - fmt.Fprintf(w, `[{"sha":%q}]`, signedCommit) - case strings.HasSuffix(r.URL.Path, "/pulls/42"): - fmt.Fprintf(w, `{"number":42,"html_url":"https://github.com/codefly-dev/manifests/pull/42","state":"closed","merged":true,"merge_commit_sha":%q}`, observedRevision) - default: - http.NotFound(w, r) - } - })) - defer server.Close() - t.Setenv("GITHUB_TOKEN", "test-token") - t.Setenv("GITHUB_API_URL", server.URL) + bin := t.TempDir() + script := filepath.Join(bin, "gh") + content := `#!/bin/sh +printf '%s\n' "$CODEFLY_TEST_GH_RESPONSE" +` + if err := os.WriteFile(script, []byte(content), 0o755); err != nil { + t.Fatal(err) + } + t.Setenv("PATH", bin+string(os.PathListSeparator)+os.Getenv("PATH")) + t.Setenv("CODEFLY_TEST_GH_RESPONSE", `{ + "url":"https://github.com/codefly-dev/manifests/pull/42", + "state":"MERGED", + "reviewDecision":"APPROVED", + "reviews":[{"state":"APPROVED","author":{"login":"reviewer"}}], + "mergeCommit":{"oid":"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"}, + "commits":[{"oid":"cccccccccccccccccccccccccccccccccccccccc"}] +}`) review, err := observeReview(context.Background(), "https://github.com/codefly-dev/manifests/pull/42", signedCommit, "https://github.com/codefly-dev/manifests.git", false) diff --git a/pkg/gitops/publish_test.go b/pkg/gitops/publish_test.go index e94fd755..a8ddabb7 100644 --- a/pkg/gitops/publish_test.go +++ b/pkg/gitops/publish_test.go @@ -3,6 +3,8 @@ package gitops import ( "context" "fmt" + "net/http" + "net/http/httptest" "os" "os/exec" "path/filepath" @@ -16,6 +18,67 @@ import ( var preparedPermit = mutationauthority.NewPreparedPermit() +func TestOpenOrUpdatePullRequestEditsExistingOpenPromotion(t *testing.T) { + const commit = "1234567890123456789012345678901234567890" + prepared := &preparedRepository{plan: PublishPlan{ + RepositorySlug: "codefly-test/manifests", + PromotionBranch: "codefly/promote-payments-aws", + BaseBranch: "main", + }} + + var edited bool + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch { + case r.Method == http.MethodGet && strings.HasSuffix(r.URL.Path, "/pulls"): + fmt.Fprintf(w, `[{"number":7,"html_url":"https://github.com/codefly-test/manifests/pull/7","head":{"sha":%q}}]`, commit) + case r.Method == http.MethodPatch && strings.HasSuffix(r.URL.Path, "/pulls/7"): + edited = true + fmt.Fprint(w, `{"number":7,"html_url":"https://github.com/codefly-test/manifests/pull/7"}`) + default: + http.Error(w, "unexpected "+r.Method+" "+r.URL.Path, http.StatusInternalServerError) + } + })) + defer server.Close() + t.Setenv("GITHUB_TOKEN", "test-token") + t.Setenv("GITHUB_API_URL", server.URL) + + url, number, err := openOrUpdatePullRequest(context.Background(), prepared, &PublishRequest{}, commit) + if err != nil { + t.Fatal(err) + } + if !edited { + t.Fatal("existing pull request was not edited") + } + if url != "https://github.com/codefly-test/manifests/pull/7" || number != 7 { + t.Fatalf("openOrUpdatePullRequest = %q, %d", url, number) + } +} + +func TestOpenOrUpdatePullRequestRejectsHeadCommitDrift(t *testing.T) { + const commit = "1234567890123456789012345678901234567890" + prepared := &preparedRepository{plan: PublishPlan{ + RepositorySlug: "codefly-test/manifests", + PromotionBranch: "codefly/promote-payments-aws", + BaseBranch: "main", + }} + + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method == http.MethodGet && strings.HasSuffix(r.URL.Path, "/pulls") { + fmt.Fprint(w, `[{"number":7,"html_url":"https://github.com/codefly-test/manifests/pull/7","head":{"sha":"ffffffffffffffffffffffffffffffffffffffff"}}]`) + return + } + http.Error(w, "unexpected "+r.Method+" "+r.URL.Path, http.StatusInternalServerError) + })) + defer server.Close() + t.Setenv("GITHUB_TOKEN", "test-token") + t.Setenv("GITHUB_API_URL", server.URL) + + if _, _, err := openOrUpdatePullRequest(context.Background(), prepared, &PublishRequest{}, commit); err == nil || + !strings.Contains(err.Error(), "pull request head is") { + t.Fatalf("head drift error = %v", err) + } +} + func TestInventoryUnitDirectoriesAreDistinctAndKindChecked(t *testing.T) { dirs, err := inventoryUnitDirectories(&Inventory{Units: []InventoryUnit{ {Kind: UnitKindService, Name: "api", Path: "services/api"}, From 2241136b1e33628c71172fb819b62cd8fb05d415 Mon Sep 17 00:00:00 2001 From: Antoine Toussaint Date: Sun, 23 Aug 2026 17:49:02 -0400 Subject: [PATCH 3/3] fix(lint): silence gosec taint on git shell-out; drop err shadow (#458) golangci-lint flagged two new issues from the go-github migration: - gosec G702 on the `git remote get-url` call in agentRepository: git runs with fixed subcommands and a scanned filesystem path as an argument (no shell), so annotate with //nolint:gosec matching the pkg/librarystore precedent. - govet shadow: the PullRequests.Edit error redeclared the outer `err`; rename it to editErr. Co-Authored-By: Claude Opus 4.8 --- cmd/status/release.go | 1 + pkg/gitops/publish.go | 6 +++--- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/cmd/status/release.go b/cmd/status/release.go index 66951414..0abfd58e 100644 --- a/cmd/status/release.go +++ b/cmd/status/release.go @@ -280,6 +280,7 @@ func createAgentIssue(baseDir string, status AgentStatus) error { } func agentRepository(agentPath string) (string, string, error) { + //nolint:gosec // git is invoked with fixed subcommands; agentPath is a scanned filesystem path, never a shell. out, err := exec.Command("git", "-C", agentPath, "remote", "get-url", "origin").Output() if err != nil { return "", "", fmt.Errorf("resolve %s origin remote: %w", agentPath, err) diff --git a/pkg/gitops/publish.go b/pkg/gitops/publish.go index c8d74b7b..ee3ecae6 100644 --- a/pkg/gitops/publish.go +++ b/pkg/gitops/publish.go @@ -1277,11 +1277,11 @@ func openOrUpdatePullRequest(ctx context.Context, prepared *preparedRepository, if pr.GetHead().GetSHA() != commit { return "", 0, fmt.Errorf("pull request head is %s, expected %s", pr.GetHead().GetSHA(), commit) } - if _, _, err := client.PullRequests.Edit(ctx, owner, repo, pr.GetNumber(), &github.PullRequest{ + if _, _, editErr := client.PullRequests.Edit(ctx, owner, repo, pr.GetNumber(), &github.PullRequest{ Title: github.Ptr(title), Body: github.Ptr(body), - }); err != nil { - return "", 0, fmt.Errorf("update promotion pull request: %w", err) + }); editErr != nil { + return "", 0, fmt.Errorf("update promotion pull request: %w", editErr) } return pr.GetHTMLURL(), pr.GetNumber(), nil }