From c8c4ede705ef4e8ae73f66cf51164a3b427bf62f Mon Sep 17 00:00:00 2001 From: avidal Date: Mon, 30 Mar 2026 11:29:20 -0500 Subject: [PATCH 1/3] feat: use GraphQL when possible, fall back to REST if necessary Prior to v3, commit-headless always used the GraphQL API to create commits. It's a single API call that replaces 3+ calls via the REST API (create blob for each file, create a tree, then create the commit). However, the GraphQL API had some limitations: - File modes are not preserved. Pushing a commit that updates a symlink or an executable file would result in the mode being reset to 100644 (plain file) - No ability to force push: you must always specify the expected head SHA and not having the correct head SHA would fail the commit So in v3 I swapped to the REST API. It's more API calls, sure, but it's more flexible. It allowed me to introduce new functionality, such as: - The `replay` command to replace an unsigned remote commit with a signed one - Force pushing for cases where you may rebase in your workflow However, this uncovered another issue: commits created via the REST API using *user tokens* are not signed (server/app tokens *are* signed). Since we have some use cases where a system is acting on behalf of a user, this was a significant regression. To address the issues with the REST API and the issues with the GraphQL API, this patch implements a hybrid approach: - Use the GraphQL API by default: less API calls means less likely to hit rate limits - Swap to the REST API if a commit contains a file with a non-standard file mode To address the issue with force pushes all commits are created on a temporary branch, branched off of the supplied `--head-sha`. Once all commits are created, regardless of strategy, the REST API is used to update the reference on the supplied target branch to point to the HEAD of the temporary branch and the temporary branch is discarded. You can think about this workflow as analogous to working on a "feature branch" and then fast-forward merging the feature branch. For force pushes or replays, it's analogous to first resetting the target branch to an earlier commit and then fast forward merging the temporary branch. This has the secondary benefits of all work being done in a staging area (the temporary branch). The target branch is left alone until the very end. Note there is one caveat: because signing is more important than file mode preservation, if a *user* token is detected, all commits will be created using GraphQL. --- .github/workflows/test.yml | 14 + README.md | 37 ++- action-template/README.md | 4 + change.go | 11 + change_test.go | 22 ++ github.go | 369 ++++++++++++++++++++---- github_test.go | 566 +++++++++++++++++++++---------------- token.go | 12 + token_test.go | 27 ++ version.go | 2 +- 10 files changed, 747 insertions(+), 317 deletions(-) create mode 100644 token_test.go diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index e0e0bd5..124ff2a 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -163,6 +163,17 @@ jobs: exit 1 fi + - &verify-no-tmp-branches + name: Verify no leftover working branches + run: | + tmp_branches=$(git ls-remote --heads origin | grep -- '--headless-tmp-' || true) + if [ -n "$tmp_branches" ]; then + echo "ERROR: leftover working branches found:" + echo "$tmp_branches" + exit 1 + fi + echo "No leftover working branches" + - *cleanup test-push-no-op: @@ -280,6 +291,7 @@ jobs: exit 1 fi + - *verify-no-tmp-branches - *cleanup test-commit-create-branch: @@ -421,6 +433,7 @@ jobs: fi echo "Executable bit preserved successfully" + - *verify-no-tmp-branches - *cleanup test-replay: @@ -498,6 +511,7 @@ jobs: echo "Replay test passed: commits were replayed with new hashes" + - *verify-no-tmp-branches - *cleanup test-replay-single: diff --git a/README.md b/README.md index 899ce1e..08f77b8 100644 --- a/README.md +++ b/README.md @@ -2,12 +2,15 @@ A binary tool and GitHub Action for creating signed commits from headless workflows. -`commit-headless` turns local commits into commits on the remote using the GitHub REST API. -When commits are created using the API with a GitHub App or installation token (such as -`github.token` in Actions), they are signed and verified by GitHub. Commits created with a personal -access token or OAuth token will not be signed. +`commit-headless` turns local commits into commits on the remote using the GitHub API. +By default, commits are created using the GraphQL `createCommitOnBranch` mutation, which produces +signed and verified commits regardless of token type. For commits that modify files with non-default +modes (e.g., executables) and a non-user token, the tool automatically falls back to the REST API +to preserve file modes. -File modes (such as the executable bit) are preserved when pushing commits. +File modes (such as the executable bit) are preserved when using the REST API path. The GraphQL API +does not support file modes — all files are treated as regular files (`100644`). For user tokens +(PAT, OAuth, `ghu_`), signing takes priority and the GraphQL path is always used. For the GitHub Action, see [the action branch][action-branch] and the associated `action/` release tags. @@ -138,12 +141,9 @@ By default, `commit-headless` verifies that each commit created via the API is s If a commit is not signed, it retries with exponential backoff (1s, 2s, 4s, ...) up to `--sign-attempts` times (default: 5). -Whether GitHub signs a commit depends on the token type: - -- **GitHub App / installation tokens** (including `github.token` in Actions): commits are signed - and verified by GitHub. -- **Personal access tokens / OAuth tokens**: commits are not signed. Set `--sign-attempts 0` to - skip verification when using these token types. +All token types can produce signed commits. The GraphQL API (used by default) produces signed +commits for both user tokens and app tokens. The REST API fallback also produces signed commits +for GitHub App / installation tokens. Even with a valid token, GitHub may occasionally fail to sign a commit. This has been observed internally and is not consistently reproducible. The retry mechanism exists as a safety net for @@ -152,6 +152,21 @@ these transient failures. If all attempts are exhausted without a signed commit, `commit-headless` exits with an error. This ensures unsigned commits are never silently pushed to the remote. +## API strategy + +`commit-headless` automatically chooses between the GraphQL and REST APIs on a per-commit basis: + +- **GraphQL** (default): Uses `createCommitOnBranch` — fewer API calls, commits are signed + server-side. Does not support file modes (all files are `100644`). +- **REST** (fallback): Uses the Git Database API — more API calls, but preserves file modes. + +The REST fallback is used when a commit modifies files with non-default modes (e.g., `100755` for +executables) and the token is not a user token. User tokens (`ghp_`, `gho_`, `ghu_`, +`github_pat_`) always use GraphQL since signing is the priority. + +The tool logs which strategy is used for each commit. If the GraphQL path is used for a commit with +non-default file modes, a warning is logged. + ## Try it Create a local commit and push it to a new branch: diff --git a/action-template/README.md b/action-template/README.md index 49afe22..9d09370 100644 --- a/action-template/README.md +++ b/action-template/README.md @@ -2,6 +2,10 @@ This action creates signed and verified commits on GitHub from a workflow. +Commits are created using the GraphQL API by default, which produces signed commits for all token +types. When a commit modifies files with non-default modes (e.g., executables) and a non-user token +is used, the action automatically falls back to the REST API to preserve file modes. + For source code and CLI documentation, see the [main branch](https://github.com/DataDog/commit-headless/tree/main). ## Commands diff --git a/change.go b/change.go index 9954ff4..baad507 100644 --- a/change.go +++ b/change.go @@ -25,6 +25,17 @@ type Change struct { entries map[string]FileEntry } +// HasNonDefaultModes returns true if any file entry has a mode other than the +// default 100644 (regular file). Empty mode is treated as 100644. +func (c Change) HasNonDefaultModes() bool { + for _, fe := range c.entries { + if fe.Mode != "" && fe.Mode != "100644" { + return true + } + } + return false +} + // Splits a commit message on the first blank line func (c Change) splitMessage() (string, string) { h, b, _ := strings.Cut(c.message, "\n\n") diff --git a/change_test.go b/change_test.go index 1a089d5..de3b88d 100644 --- a/change_test.go +++ b/change_test.go @@ -5,6 +5,28 @@ import ( "testing" ) +func TestHasNonDefaultModes(t *testing.T) { + tests := []struct { + name string + entries map[string]FileEntry + want bool + }{ + {"all default", map[string]FileEntry{"a": {Mode: "100644"}}, false}, + {"empty mode treated as default", map[string]FileEntry{"a": {Mode: ""}}, false}, + {"executable", map[string]FileEntry{"a": {Mode: "100755"}}, true}, + {"symlink", map[string]FileEntry{"a": {Mode: "120000"}}, true}, + {"mixed", map[string]FileEntry{"a": {Mode: "100644"}, "b": {Mode: "100755"}}, true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + c := Change{entries: tt.entries} + if got := c.HasNonDefaultModes(); got != tt.want { + t.Errorf("HasNonDefaultModes() = %v, want %v", got, tt.want) + } + }) + } +} + func TestChangeBody(t *testing.T) { testcases := []struct { input string diff --git a/github.go b/github.go index 08b52c9..604de30 100644 --- a/github.go +++ b/github.go @@ -1,9 +1,13 @@ package main import ( + "bytes" "context" + "encoding/base64" + "encoding/json" "errors" "fmt" + "math/rand/v2" "net/http" "strings" "time" @@ -27,16 +31,73 @@ type GitAPI interface { CreateTree(ctx context.Context, owner, repo, baseTree string, entries []*github.TreeEntry) (*github.Tree, *github.Response, error) CreateCommit(ctx context.Context, owner, repo string, commit github.Commit, opts *github.CreateCommitOptions) (*github.Commit, *github.Response, error) UpdateRef(ctx context.Context, owner, repo, ref string, updateRef github.UpdateRef) (*github.Reference, *github.Response, error) + DeleteRef(ctx context.Context, owner, repo, ref string) (*github.Response, error) +} + +// GraphQLAPI abstracts the GraphQL endpoint for testing. +type GraphQLAPI interface { + Do(ctx context.Context, query string, variables map[string]any) (json.RawMessage, error) +} + +// graphQLClient is the production implementation of GraphQLAPI using raw HTTP. +type graphQLClient struct { + httpClient *http.Client + endpoint string +} + +func (g *graphQLClient) Do(ctx context.Context, query string, variables map[string]any) (json.RawMessage, error) { + body := map[string]any{ + "query": query, + "variables": variables, + } + buf, err := json.Marshal(body) + if err != nil { + return nil, fmt.Errorf("marshal graphql request: %w", err) + } + + req, err := http.NewRequestWithContext(ctx, http.MethodPost, g.endpoint, bytes.NewReader(buf)) + if err != nil { + return nil, fmt.Errorf("create graphql request: %w", err) + } + req.Header.Set("Content-Type", "application/json") + + resp, err := g.httpClient.Do(req) + if err != nil { + return nil, fmt.Errorf("graphql request: %w", err) + } + defer resp.Body.Close() + + var result struct { + Data json.RawMessage `json:"data"` + Errors []struct { + Message string `json:"message"` + } `json:"errors"` + } + if err := json.NewDecoder(resp.Body).Decode(&result); err != nil { + return nil, fmt.Errorf("decode graphql response: %w", err) + } + + if len(result.Errors) > 0 { + msgs := make([]string, len(result.Errors)) + for i, e := range result.Errors { + msgs[i] = e.Message + } + return nil, fmt.Errorf("graphql errors: %s", strings.Join(msgs, "; ")) + } + + return result.Data, nil } // Client provides methods for interacting with a remote repository on GitHub type Client struct { - repos RepositoriesAPI - git GitAPI - owner string - repo string - branch string - + repos RepositoriesAPI + git GitAPI + graphql GraphQLAPI + owner string + repo string + branch string + + userToken bool dryrun bool force bool signAttempts int @@ -50,11 +111,16 @@ func NewClient(ctx context.Context, token, owner, repo, branch string) *Client { ghClient := github.NewClient(httpC) return &Client{ - repos: ghClient.Repositories, - git: ghClient.Git, - owner: owner, - repo: repo, - branch: branch, + repos: ghClient.Repositories, + git: ghClient.Git, + graphql: &graphQLClient{ + httpClient: httpC, + endpoint: "https://api.github.com/graphql", + }, + owner: owner, + repo: repo, + branch: branch, + userToken: isUserToken(token), } } @@ -97,36 +163,106 @@ func (c *Client) CreateBranch(ctx context.Context, headSha string) (string, erro return created.GetObject().GetSHA(), nil } -// PushChanges creates commits for each change, then updates the branch ref once at the end. -// This is all-or-nothing: if any commit fails, the branch ref is not updated. +// commitStrategy indicates which API to use for creating a commit. +type commitStrategy int + +const ( + strategyGraphQL commitStrategy = iota + strategyREST +) + +func (s commitStrategy) String() string { + if s == strategyGraphQL { + return "GraphQL" + } + return "REST" +} + +// chooseStrategy decides whether to use GraphQL or REST for a given change. +func (c *Client) chooseStrategy(change Change) commitStrategy { + if change.HasNonDefaultModes() && !c.userToken { + return strategyREST + } + return strategyGraphQL +} + +// PushChanges creates commits for each change on a throwaway branch, then updates the real branch +// ref once at the end. The throwaway branch is always cleaned up on completion. // It returns the number of changes that were successfully pushed, the new head reference hash, and // any error encountered. func (c *Client) PushChanges(ctx context.Context, headCommit string, changes ...Change) (int, string, error) { - var err error - for i, change := range changes { - headCommit, err = c.CreateChange(ctx, headCommit, change) - if err != nil { - return i + 1, "", fmt.Errorf("push change %d: %w", i+i, err) + if c.dryrun { + for i, change := range changes { + _, err := c.CreateChange(ctx, "", headCommit, change) + if err != nil { + return i + 1, "", fmt.Errorf("push change %d: %w", i, err) + } } + return len(changes), strings.Repeat("0", 40), nil + } + + // Create throwaway branch + tmpBranch := fmt.Sprintf("%s--headless-tmp-%s", c.branch, randomSuffix()) + tmpRef := fmt.Sprintf("refs/heads/%s", tmpBranch) + + logger.Printf("Creating working branch %s\n", tmpBranch) + _, _, err := c.git.CreateRef(ctx, c.owner, c.repo, github.CreateRef{ + Ref: tmpRef, + SHA: headCommit, + }) + if err != nil { + return 0, "", fmt.Errorf("create working branch: %w", err) } - if !c.dryrun { - _, _, err = c.git.UpdateRef(ctx, c.owner, c.repo, "refs/heads/"+c.branch, github.UpdateRef{ - SHA: headCommit, - Force: github.Ptr(c.force), - }) + // Always clean up the throwaway branch + defer func() { + logger.Printf("Cleaning up working branch %s\n", tmpBranch) + if _, err := c.git.DeleteRef(ctx, c.owner, c.repo, tmpRef); err != nil { + logger.Warningf("Failed to delete working branch %s: %v", tmpBranch, err) + } + }() + + // Create commits on the throwaway branch + for i, change := range changes { + newHead, err := c.CreateChange(ctx, tmpBranch, headCommit, change) if err != nil { - return len(changes), "", fmt.Errorf("update ref: %w", err) + return i + 1, "", fmt.Errorf("push change %d: %w", i, err) } + + strategy := c.chooseStrategy(change) + if strategy == strategyREST { + // REST creates a detached commit; advance the throwaway branch to stay in sync + _, _, err = c.git.UpdateRef(ctx, c.owner, c.repo, tmpRef, github.UpdateRef{ + SHA: newHead, + Force: github.Ptr(true), + }) + if err != nil { + return i + 1, "", fmt.Errorf("advance working branch: %w", err) + } + } + + headCommit = newHead + } + + // Update the real branch + _, _, err = c.git.UpdateRef(ctx, c.owner, c.repo, "refs/heads/"+c.branch, github.UpdateRef{ + SHA: headCommit, + Force: github.Ptr(c.force), + }) + if err != nil { + return len(changes), "", fmt.Errorf("update ref: %w", err) } return len(changes), headCommit, nil } -// CreateChange creates a single commit from a change using the REST API. -// It does not update the branch ref — that is done by PushChanges after all commits succeed. -// It returns the hash of the created commit or an error. -func (c *Client) CreateChange(ctx context.Context, headCommit string, change Change) (string, error) { +// CreateChange creates a single commit from a change, dispatching to GraphQL or REST based on +// the change contents and token type. It handles logging, dry-run, and signature verification +// retry for both strategies. +// +// tmpBranch is the name of the throwaway branch (without refs/heads/ prefix) used for GraphQL +// commits. It may be empty for dry-run mode. +func (c *Client) CreateChange(ctx context.Context, tmpBranch, headCommit string, change Change) (string, error) { shortHash := change.hash if len(shortHash) > 8 { shortHash = shortHash[:8] @@ -150,22 +286,76 @@ func (c *Client) CreateChange(ctx context.Context, headCommit string, change Cha logger.Printf(" - %s: %s\n", action, path) } + strategy := c.chooseStrategy(change) + logger.Printf("Strategy: %s\n", strategy) + + if strategy == strategyGraphQL && change.HasNonDefaultModes() { + logger.Warningf("GraphQL API does not preserve file modes; non-default modes in this commit will be reset to 100644") + } + if c.dryrun { logger.Notice("Dry run enabled, not writing commit") return strings.Repeat("0", len(change.hash)), nil } - // Get the parent commit's tree SHA + // Build the commit function based on strategy + var commitFn func() (string, bool, error) + + switch strategy { + case strategyGraphQL: + commitFn = func() (string, bool, error) { + return c.execGraphQLCommit(ctx, tmpBranch, headCommit, change) + } + case strategyREST: + // Prep tree once before the retry loop + treeSHA, err := c.prepTree(ctx, headCommit, change) + if err != nil { + return "", err + } + commitFn = func() (string, bool, error) { + return c.execRESTCommit(ctx, headCommit, treeSHA, change) + } + } + + // Execute with signature verification retry + commitSha, verified, err := commitFn() + if err != nil { + return "", fmt.Errorf("create commit: %w", err) + } + + if c.signAttempts > 0 { + backoff := 1 * time.Second + for attempt := 1; attempt <= c.signAttempts && !verified; attempt++ { + if attempt == c.signAttempts { + return "", fmt.Errorf("commit %s was not signed after %d attempt(s)", commitSha, c.signAttempts) + } + + logger.Warningf("Commit %s not signed (attempt %d/%d), retrying in %s...", commitSha, attempt, c.signAttempts, backoff) + time.Sleep(backoff) + backoff *= 2 + + commitSha, verified, err = commitFn() + if err != nil { + return "", fmt.Errorf("create commit (attempt %d): %w", attempt+1, err) + } + } + } + + logger.Printf("Created: %s\n", c.commitURL(commitSha)) + return commitSha, nil +} + +// prepTree creates blobs and a tree for the REST commit path. This is called once before the +// retry loop since the tree is immutable and can be reused across retries. +func (c *Client) prepTree(ctx context.Context, headCommit string, change Change) (string, error) { parentCommit, _, err := c.git.GetCommit(ctx, c.owner, c.repo, headCommit) if err != nil { return "", fmt.Errorf("get parent commit: %w", err) } baseTreeSHA := parentCommit.GetTree().GetSHA() - // Build tree entries var entries []*github.TreeEntry for path, fe := range change.entries { - // Use the file's mode, defaulting to 100644 for regular files mode := fe.Mode if mode == "" { mode = "100644" @@ -179,7 +369,6 @@ func (c *Client) CreateChange(ctx context.Context, headCommit string, change Cha if fe.Content == nil { // Deletion: SHA must be empty string for go-github to omit it } else { - // Create blob for additions/modifications blob, _, err := c.git.CreateBlob(ctx, c.owner, c.repo, github.Blob{ Content: github.Ptr(string(fe.Content)), Encoding: github.Ptr("utf-8"), @@ -192,13 +381,16 @@ func (c *Client) CreateChange(ctx context.Context, headCommit string, change Cha entries = append(entries, entry) } - // Create tree tree, _, err := c.git.CreateTree(ctx, c.owner, c.repo, baseTreeSHA, entries) if err != nil { return "", fmt.Errorf("create tree: %w", err) } - // Create commit (with signature verification retry) + return tree.GetSHA(), nil +} + +// execRESTCommit creates a commit using the REST API with a pre-built tree. +func (c *Client) execRESTCommit(ctx context.Context, headCommit, treeSHA string, change Change) (string, bool, error) { message := change.Headline() if body := change.Body(); body != "" { message = message + "\n\n" + body @@ -206,40 +398,105 @@ func (c *Client) CreateChange(ctx context.Context, headCommit string, change Cha commitInput := github.Commit{ Message: github.Ptr(message), - Tree: &github.Tree{SHA: tree.SHA}, + Tree: &github.Tree{SHA: github.Ptr(treeSHA)}, Parents: []*github.Commit{{SHA: github.Ptr(headCommit)}}, } commit, _, err := c.git.CreateCommit(ctx, c.owner, c.repo, commitInput, nil) if err != nil { - return "", fmt.Errorf("create commit: %w", err) + return "", false, fmt.Errorf("REST create commit: %w", err) } - if c.signAttempts > 0 { - backoff := 1 * time.Second - for attempt := 1; attempt <= c.signAttempts; attempt++ { - if commit.GetVerification().GetVerified() { - break - } + return commit.GetSHA(), commit.GetVerification().GetVerified(), nil +} - if attempt == c.signAttempts { - reason := commit.GetVerification().GetReason() - return "", fmt.Errorf("commit %s was not signed after %d attempt(s) (reason: %s)", commit.GetSHA(), c.signAttempts, reason) - } +// createCommitOnBranch GraphQL mutation +const createCommitOnBranchMutation = ` +mutation CreateCommitOnBranch($input: CreateCommitOnBranchInput!) { + createCommitOnBranch(input: $input) { + commit { + oid + signature { + isValid + } + } + } +} +` - logger.Warningf("Commit %s not signed (attempt %d/%d, reason: %s), retrying in %s...", commit.GetSHA(), attempt, c.signAttempts, commit.GetVerification().GetReason(), backoff) - time.Sleep(backoff) - backoff *= 2 +// execGraphQLCommit creates a commit using the GraphQL createCommitOnBranch mutation. +func (c *Client) execGraphQLCommit(ctx context.Context, tmpBranch, headCommit string, change Change) (string, bool, error) { + // Build file additions and deletions + var additions []map[string]string + var deletions []map[string]string - commit, _, err = c.git.CreateCommit(ctx, c.owner, c.repo, commitInput, nil) - if err != nil { - return "", fmt.Errorf("create commit (attempt %d): %w", attempt+1, err) - } + for path, fe := range change.entries { + if fe.Content == nil { + deletions = append(deletions, map[string]string{"path": path}) + } else { + additions = append(additions, map[string]string{ + "path": path, + "contents": base64.StdEncoding.EncodeToString(fe.Content), + }) } } - commitSha := commit.GetSHA() - logger.Printf("Created: %s\n", c.commitURL(commitSha)) + headline := change.Headline() + body := change.Body() - return commitSha, nil + message := map[string]string{"headline": headline} + if body != "" { + message["body"] = body + } + + fileChanges := map[string]any{} + if len(additions) > 0 { + fileChanges["additions"] = additions + } + if len(deletions) > 0 { + fileChanges["deletions"] = deletions + } + + input := map[string]any{ + "branch": map[string]any{ + "repositoryNameWithOwner": fmt.Sprintf("%s/%s", c.owner, c.repo), + "branchName": tmpBranch, + }, + "expectedHeadOid": headCommit, + "message": message, + "fileChanges": fileChanges, + } + + data, err := c.graphql.Do(ctx, createCommitOnBranchMutation, map[string]any{"input": input}) + if err != nil { + return "", false, fmt.Errorf("GraphQL createCommitOnBranch: %w", err) + } + + var result struct { + CreateCommitOnBranch struct { + Commit struct { + OID string `json:"oid"` + Signature struct { + IsValid bool `json:"isValid"` + } `json:"signature"` + } `json:"commit"` + } `json:"createCommitOnBranch"` + } + if err := json.Unmarshal(data, &result); err != nil { + return "", false, fmt.Errorf("unmarshal GraphQL response: %w", err) + } + + oid := result.CreateCommitOnBranch.Commit.OID + signed := result.CreateCommitOnBranch.Commit.Signature.IsValid + + return oid, signed, nil +} + +func randomSuffix() string { + const chars = "abcdefghijklmnopqrstuvwxyz0123456789" + b := make([]byte, 8) + for i := range b { + b[i] = chars[rand.IntN(len(chars))] + } + return string(b) } diff --git a/github_test.go b/github_test.go index e70735d..dc9c50a 100644 --- a/github_test.go +++ b/github_test.go @@ -3,6 +3,7 @@ package main import ( "context" "encoding/json" + "fmt" "io" "net/http" "net/http/httptest" @@ -22,6 +23,23 @@ func newFileEntry(content []byte) FileEntry { return FileEntry{Content: content, Mode: "100644"} } +// mockGraphQL implements GraphQLAPI for testing. +type mockGraphQL struct { + handler func(query string, variables map[string]any) (json.RawMessage, error) +} + +func (m *mockGraphQL) Do(_ context.Context, query string, variables map[string]any) (json.RawMessage, error) { + return m.handler(query, variables) +} + +func signedGraphQLResponse(oid string) json.RawMessage { + return json.RawMessage(`{"createCommitOnBranch":{"commit":{"oid":"` + oid + `","signature":{"isValid":true}}}}`) +} + +func unsignedGraphQLResponse(oid string) json.RawMessage { + return json.RawMessage(`{"createCommitOnBranch":{"commit":{"oid":"` + oid + `","signature":{"isValid":false}}}}`) +} + func TestBuildTreeEntries(t *testing.T) { change := Change{ entries: map[string]FileEntry{ @@ -31,7 +49,7 @@ func TestBuildTreeEntries(t *testing.T) { }, } - // Build tree entries the same way CreateChange does (without making API calls) + // Build tree entries the same way prepTree does (without making API calls) var addedPaths, deletedPaths []string for path, fe := range change.entries { if fe.Content == nil { @@ -207,17 +225,58 @@ func TestCreateChange(t *testing.T) { }, } - sha, err := client.CreateChange(context.Background(), "head-sha", change) + sha, err := client.CreateChange(context.Background(), "", "head-sha", change) if err != nil { t.Fatalf("unexpected error: %v", err) } - // Should return zeros with same length as input hash if sha != "000000" { t.Errorf("expected '000000', got %q", sha) } }) - t.Run("successful push with file addition", func(t *testing.T) { + t.Run("graphql commit", func(t *testing.T) { + callCount := 0 + client := &Client{ + owner: "test-owner", + repo: "test-repo", + branch: "test-branch", + graphql: &mockGraphQL{ + handler: func(query string, variables map[string]any) (json.RawMessage, error) { + callCount++ + input := variables["input"].(map[string]any) + branch := input["branch"].(map[string]any) + if branch["branchName"] != "tmp-branch" { + t.Errorf("expected branch 'tmp-branch', got %v", branch["branchName"]) + } + if input["expectedHeadOid"] != "parent-sha" { + t.Errorf("expected expectedHeadOid 'parent-sha', got %v", input["expectedHeadOid"]) + } + return signedGraphQLResponse("new-commit-sha"), nil + }, + }, + } + + change := Change{ + hash: "local-hash", + message: "Test commit", + entries: map[string]FileEntry{ + "file.txt": newFileEntry([]byte("content")), + }, + } + + sha, err := client.CreateChange(context.Background(), "tmp-branch", "parent-sha", change) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if sha != "new-commit-sha" { + t.Errorf("expected 'new-commit-sha', got %q", sha) + } + if callCount != 1 { + t.Errorf("expected 1 GraphQL call, got %d", callCount) + } + }) + + t.Run("rest fallback for non-default modes with non-user token", func(t *testing.T) { var ( getCommitCalled bool createBlobCalled bool @@ -240,8 +299,6 @@ func TestCreateChange(t *testing.T) { case r.Method == http.MethodPost && r.URL.Path == "/repos/test-owner/test-repo/git/blobs": createBlobCalled = true - var blob github.Blob - json.NewDecoder(r.Body).Decode(&blob) w.WriteHeader(http.StatusCreated) json.NewEncoder(w).Encode(github.Blob{ SHA: github.Ptr("blob-sha-123"), @@ -256,11 +313,6 @@ func TestCreateChange(t *testing.T) { case r.Method == http.MethodPost && r.URL.Path == "/repos/test-owner/test-repo/git/commits": commitCalled = true - var commit github.Commit - json.NewDecoder(r.Body).Decode(&commit) - if commit.GetMessage() != "Test commit" { - t.Errorf("unexpected commit message: %s", commit.GetMessage()) - } w.WriteHeader(http.StatusCreated) json.NewEncoder(w).Encode(github.Commit{ SHA: github.Ptr("new-commit-sha"), @@ -274,15 +326,17 @@ func TestCreateChange(t *testing.T) { defer server.Close() client := newTestClient(t, server) + client.userToken = false // non-user token + change := Change{ hash: "local-hash", message: "Test commit", entries: map[string]FileEntry{ - "file.txt": newFileEntry([]byte("content")), + "script.sh": {Content: []byte("#!/bin/bash"), Mode: "100755"}, }, } - sha, err := client.CreateChange(context.Background(), "parent-sha", change) + sha, err := client.CreateChange(context.Background(), "tmp-branch", "parent-sha", change) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -304,149 +358,90 @@ func TestCreateChange(t *testing.T) { } }) - t.Run("successful push with file deletion", func(t *testing.T) { - var treeEntries []*github.TreeEntry - - server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - w.Header().Set("Content-Type", "application/json") - - switch { - case r.Method == http.MethodGet && strings.HasPrefix(r.URL.Path, "/repos/test-owner/test-repo/git/commits/"): - json.NewEncoder(w).Encode(github.Commit{ - SHA: github.Ptr("parent-sha"), - Tree: &github.Tree{ - SHA: github.Ptr("parent-tree-sha"), - }, - }) - - case r.Method == http.MethodPost && r.URL.Path == "/repos/test-owner/test-repo/git/trees": - var req struct { - BaseTree string `json:"base_tree"` - Tree []*github.TreeEntry `json:"tree"` - } - json.NewDecoder(r.Body).Decode(&req) - treeEntries = req.Tree - w.WriteHeader(http.StatusCreated) - json.NewEncoder(w).Encode(github.Tree{ - SHA: github.Ptr("new-tree-sha"), - }) - - case r.Method == http.MethodPost && r.URL.Path == "/repos/test-owner/test-repo/git/commits": - w.WriteHeader(http.StatusCreated) - json.NewEncoder(w).Encode(github.Commit{ - SHA: github.Ptr("new-commit-sha"), - }) - - case r.Method == http.MethodPatch && strings.HasPrefix(r.URL.Path, "/repos/test-owner/test-repo/git/refs/"): - json.NewEncoder(w).Encode(github.Reference{ - Object: &github.GitObject{ - SHA: github.Ptr("new-commit-sha"), - }, - }) - - default: - t.Errorf("unexpected request: %s %s", r.Method, r.URL.Path) - w.WriteHeader(http.StatusNotFound) - } - })) - defer server.Close() + t.Run("graphql used for non-default modes with user token", func(t *testing.T) { + client := &Client{ + owner: "test-owner", + repo: "test-repo", + branch: "test-branch", + userToken: true, + graphql: &mockGraphQL{ + handler: func(query string, variables map[string]any) (json.RawMessage, error) { + return signedGraphQLResponse("graphql-commit-sha"), nil + }, + }, + } - client := newTestClient(t, server) change := Change{ hash: "local-hash", - message: "Delete file", + message: "Test commit", entries: map[string]FileEntry{ - "deleted-file.txt": {Content: nil}, // deletion + "script.sh": {Content: []byte("#!/bin/bash"), Mode: "100755"}, }, } - _, err := client.CreateChange(context.Background(), "parent-sha", change) + sha, err := client.CreateChange(context.Background(), "tmp-branch", "parent-sha", change) if err != nil { t.Fatalf("unexpected error: %v", err) } - - // Verify that the tree entry for deletion has no SHA (which signals deletion) - if len(treeEntries) != 1 { - t.Fatalf("expected 1 tree entry, got %d", len(treeEntries)) - } - if treeEntries[0].GetPath() != "deleted-file.txt" { - t.Errorf("expected path 'deleted-file.txt', got %q", treeEntries[0].GetPath()) - } - // For deletions, SHA should be nil/empty - if treeEntries[0].SHA != nil && *treeEntries[0].SHA != "" { - t.Errorf("expected nil/empty SHA for deletion, got %q", *treeEntries[0].SHA) + if sha != "graphql-commit-sha" { + t.Errorf("expected 'graphql-commit-sha', got %q", sha) } }) - t.Run("commit with body", func(t *testing.T) { - var receivedMessage string - - server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - w.Header().Set("Content-Type", "application/json") - - switch { - case r.Method == http.MethodGet && strings.HasPrefix(r.URL.Path, "/repos/test-owner/test-repo/git/commits/"): - json.NewEncoder(w).Encode(github.Commit{ - Tree: &github.Tree{SHA: github.Ptr("tree-sha")}, - }) - - case r.Method == http.MethodPost && r.URL.Path == "/repos/test-owner/test-repo/git/blobs": - w.WriteHeader(http.StatusCreated) - json.NewEncoder(w).Encode(github.Blob{SHA: github.Ptr("blob-sha")}) - - case r.Method == http.MethodPost && r.URL.Path == "/repos/test-owner/test-repo/git/trees": - w.WriteHeader(http.StatusCreated) - json.NewEncoder(w).Encode(github.Tree{SHA: github.Ptr("tree-sha")}) - - case r.Method == http.MethodPost && r.URL.Path == "/repos/test-owner/test-repo/git/commits": - var commit github.Commit - json.NewDecoder(r.Body).Decode(&commit) - receivedMessage = commit.GetMessage() - w.WriteHeader(http.StatusCreated) - json.NewEncoder(w).Encode(github.Commit{SHA: github.Ptr("commit-sha")}) + t.Run("commit with body via graphql", func(t *testing.T) { + var receivedHeadline, receivedBody string - case r.Method == http.MethodPatch: - json.NewEncoder(w).Encode(github.Reference{ - Object: &github.GitObject{SHA: github.Ptr("commit-sha")}, - }) - } - })) - defer server.Close() + client := &Client{ + owner: "test-owner", + repo: "test-repo", + branch: "test-branch", + graphql: &mockGraphQL{ + handler: func(query string, variables map[string]any) (json.RawMessage, error) { + input := variables["input"].(map[string]any) + msg := input["message"].(map[string]string) + receivedHeadline = msg["headline"] + if b, ok := msg["body"]; ok { + receivedBody = b + } + return signedGraphQLResponse("commit-sha"), nil + }, + }, + } - client := newTestClient(t, server) change := Change{ hash: "local", message: "Headline\n\nThis is the body\nwith multiple lines", entries: map[string]FileEntry{"file.txt": newFileEntry([]byte("x"))}, } - _, err := client.CreateChange(context.Background(), "parent", change) + _, err := client.CreateChange(context.Background(), "tmp", "parent", change) if err != nil { t.Fatalf("unexpected error: %v", err) } - if !strings.Contains(receivedMessage, "Headline") { - t.Errorf("message should contain headline, got %q", receivedMessage) + if receivedHeadline != "Headline" { + t.Errorf("expected headline 'Headline', got %q", receivedHeadline) } - if !strings.Contains(receivedMessage, "This is the body") { - t.Errorf("message should contain body, got %q", receivedMessage) + if !strings.Contains(receivedBody, "This is the body") { + t.Errorf("expected body to contain 'This is the body', got %q", receivedBody) } }) - t.Run("get parent commit fails", func(t *testing.T) { + t.Run("rest get parent commit fails", func(t *testing.T) { server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusNotFound) })) defer server.Close() client := newTestClient(t, server) + client.userToken = false change := Change{ hash: "local", message: "Test", - entries: map[string]FileEntry{"file.txt": newFileEntry([]byte("x"))}, + entries: map[string]FileEntry{"script.sh": {Content: []byte("x"), Mode: "100755"}}, } - _, err := client.CreateChange(context.Background(), "nonexistent", change) + _, err := client.CreateChange(context.Background(), "tmp", "nonexistent", change) if err == nil { t.Fatal("expected error, got nil") } @@ -454,201 +449,222 @@ func TestCreateChange(t *testing.T) { t.Errorf("expected 'get parent commit' error, got %q", err.Error()) } }) +} + +func TestPushChanges(t *testing.T) { + t.Run("graphql with throwaway branch", func(t *testing.T) { + var ( + createdRefs []string + deletedRefs []string + updatedRefs []string + ) + commitCount := 0 - t.Run("create blob fails", func(t *testing.T) { server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.Header().Set("Content-Type", "application/json") - if strings.HasPrefix(r.URL.Path, "/repos/test-owner/test-repo/git/commits/") { - json.NewEncoder(w).Encode(github.Commit{ - Tree: &github.Tree{SHA: github.Ptr("tree-sha")}, + switch { + case r.Method == http.MethodPost && r.URL.Path == "/repos/test-owner/test-repo/git/refs": + var req github.CreateRef + json.NewDecoder(r.Body).Decode(&req) + createdRefs = append(createdRefs, req.Ref) + w.WriteHeader(http.StatusCreated) + json.NewEncoder(w).Encode(github.Reference{ + Object: &github.GitObject{SHA: github.Ptr("initial")}, }) - return - } - if r.URL.Path == "/repos/test-owner/test-repo/git/blobs" { - w.WriteHeader(http.StatusInternalServerError) - return - } - })) - defer server.Close() - client := newTestClient(t, server) - change := Change{ - hash: "local", - message: "Test", - entries: map[string]FileEntry{"file.txt": newFileEntry([]byte("x"))}, - } - - _, err := client.CreateChange(context.Background(), "parent", change) - if err == nil { - t.Fatal("expected error, got nil") - } - if !strings.Contains(err.Error(), "create blob") { - t.Errorf("expected 'create blob' error, got %q", err.Error()) - } - }) + case r.Method == http.MethodPatch && strings.HasPrefix(r.URL.Path, "/repos/test-owner/test-repo/git/refs/"): + ref := strings.TrimPrefix(r.URL.Path, "/repos/test-owner/test-repo/git/refs/") + updatedRefs = append(updatedRefs, ref) + json.NewEncoder(w).Encode(github.Reference{ + Object: &github.GitObject{SHA: github.Ptr("final-sha")}, + }) - t.Run("create tree fails", func(t *testing.T) { - server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - w.Header().Set("Content-Type", "application/json") + case r.Method == http.MethodDelete && strings.HasPrefix(r.URL.Path, "/repos/test-owner/test-repo/git/refs/"): + ref := strings.TrimPrefix(r.URL.Path, "/repos/test-owner/test-repo/git/refs/") + deletedRefs = append(deletedRefs, ref) + w.WriteHeader(http.StatusNoContent) - switch { - case strings.HasPrefix(r.URL.Path, "/repos/test-owner/test-repo/git/commits/"): - json.NewEncoder(w).Encode(github.Commit{ - Tree: &github.Tree{SHA: github.Ptr("tree-sha")}, - }) - case r.URL.Path == "/repos/test-owner/test-repo/git/blobs": - w.WriteHeader(http.StatusCreated) - json.NewEncoder(w).Encode(github.Blob{SHA: github.Ptr("blob-sha")}) - case r.URL.Path == "/repos/test-owner/test-repo/git/trees": - w.WriteHeader(http.StatusInternalServerError) + default: + t.Errorf("unexpected request: %s %s", r.Method, r.URL.Path) + w.WriteHeader(http.StatusNotFound) } })) defer server.Close() client := newTestClient(t, server) - change := Change{ - hash: "local", - message: "Test", - entries: map[string]FileEntry{"file.txt": newFileEntry([]byte("x"))}, + client.graphql = &mockGraphQL{ + handler: func(query string, variables map[string]any) (json.RawMessage, error) { + commitCount++ + return signedGraphQLResponse("commit-sha-" + string(rune('0'+commitCount))), nil + }, } - _, err := client.CreateChange(context.Background(), "parent", change) - if err == nil { - t.Fatal("expected error, got nil") - } - if !strings.Contains(err.Error(), "create tree") { - t.Errorf("expected 'create tree' error, got %q", err.Error()) + changes := []Change{ + {hash: "h1", message: "First", entries: map[string]FileEntry{"a.txt": newFileEntry([]byte("a"))}}, + {hash: "h2", message: "Second", entries: map[string]FileEntry{"b.txt": newFileEntry([]byte("b"))}}, } - }) - t.Run("create commit fails", func(t *testing.T) { - server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - w.Header().Set("Content-Type", "application/json") + count, sha, err := client.PushChanges(context.Background(), "initial", changes...) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if count != 2 { + t.Errorf("expected count 2, got %d", count) + } + if sha == "" { + t.Error("expected non-empty sha") + } - switch { - case strings.HasPrefix(r.URL.Path, "/repos/test-owner/test-repo/git/commits/"): - json.NewEncoder(w).Encode(github.Commit{ - Tree: &github.Tree{SHA: github.Ptr("tree-sha")}, - }) - case r.URL.Path == "/repos/test-owner/test-repo/git/blobs": - w.WriteHeader(http.StatusCreated) - json.NewEncoder(w).Encode(github.Blob{SHA: github.Ptr("blob-sha")}) - case r.URL.Path == "/repos/test-owner/test-repo/git/trees": - w.WriteHeader(http.StatusCreated) - json.NewEncoder(w).Encode(github.Tree{SHA: github.Ptr("tree-sha")}) - case r.Method == http.MethodPost && r.URL.Path == "/repos/test-owner/test-repo/git/commits": - w.WriteHeader(http.StatusInternalServerError) - } - })) - defer server.Close() + // Should have created a throwaway branch + if len(createdRefs) != 1 { + t.Fatalf("expected 1 created ref, got %d", len(createdRefs)) + } + if !strings.Contains(createdRefs[0], "--headless-tmp-") { + t.Errorf("expected throwaway branch ref, got %q", createdRefs[0]) + } - client := newTestClient(t, server) - change := Change{ - hash: "local", - message: "Test", - entries: map[string]FileEntry{"file.txt": newFileEntry([]byte("x"))}, + // Should have updated only the real branch + if len(updatedRefs) != 1 { + t.Fatalf("expected 1 updated ref, got %d: %v", len(updatedRefs), updatedRefs) + } + if updatedRefs[0] != "heads/test-branch" { + t.Errorf("expected real branch update, got %q", updatedRefs[0]) } - _, err := client.CreateChange(context.Background(), "parent", change) - if err == nil { - t.Fatal("expected error, got nil") + // Should have deleted the throwaway branch + if len(deletedRefs) != 1 { + t.Fatalf("expected 1 deleted ref, got %d", len(deletedRefs)) + } + if !strings.Contains(deletedRefs[0], "--headless-tmp-") { + t.Errorf("expected throwaway branch deletion, got %q", deletedRefs[0]) } - if !strings.Contains(err.Error(), "create commit") { - t.Errorf("expected 'create commit' error, got %q", err.Error()) + + if commitCount != 2 { + t.Errorf("expected 2 GraphQL commits, got %d", commitCount) } }) -} + t.Run("rest fallback advances throwaway branch", func(t *testing.T) { + var updatedRefs []string -func TestPushChanges(t *testing.T) { - t.Run("multiple changes", func(t *testing.T) { - commitCount := 0 server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.Header().Set("Content-Type", "application/json") switch { - case strings.HasPrefix(r.URL.Path, "/repos/test-owner/test-repo/git/commits/"): + case r.Method == http.MethodPost && r.URL.Path == "/repos/test-owner/test-repo/git/refs": + w.WriteHeader(http.StatusCreated) + json.NewEncoder(w).Encode(github.Reference{ + Object: &github.GitObject{SHA: github.Ptr("initial")}, + }) + + case r.Method == http.MethodGet && strings.HasPrefix(r.URL.Path, "/repos/test-owner/test-repo/git/commits/"): json.NewEncoder(w).Encode(github.Commit{ Tree: &github.Tree{SHA: github.Ptr("tree-sha")}, }) + case r.URL.Path == "/repos/test-owner/test-repo/git/blobs": w.WriteHeader(http.StatusCreated) json.NewEncoder(w).Encode(github.Blob{SHA: github.Ptr("blob-sha")}) - case r.URL.Path == "/repos/test-owner/test-repo/git/trees": + + case r.Method == http.MethodPost && r.URL.Path == "/repos/test-owner/test-repo/git/trees": w.WriteHeader(http.StatusCreated) json.NewEncoder(w).Encode(github.Tree{SHA: github.Ptr("tree-sha")}) + case r.Method == http.MethodPost && r.URL.Path == "/repos/test-owner/test-repo/git/commits": - commitCount++ w.WriteHeader(http.StatusCreated) - json.NewEncoder(w).Encode(github.Commit{SHA: github.Ptr("commit-sha-" + string(rune('0'+commitCount)))}) - case r.Method == http.MethodPatch: + json.NewEncoder(w).Encode(github.Commit{SHA: github.Ptr("rest-commit-sha")}) + + case r.Method == http.MethodPatch && strings.HasPrefix(r.URL.Path, "/repos/test-owner/test-repo/git/refs/"): + ref := strings.TrimPrefix(r.URL.Path, "/repos/test-owner/test-repo/git/refs/") + updatedRefs = append(updatedRefs, ref) json.NewEncoder(w).Encode(github.Reference{ - Object: &github.GitObject{SHA: github.Ptr("final-sha")}, + Object: &github.GitObject{SHA: github.Ptr("rest-commit-sha")}, }) + + case r.Method == http.MethodDelete: + w.WriteHeader(http.StatusNoContent) + + default: + t.Errorf("unexpected request: %s %s", r.Method, r.URL.Path) + w.WriteHeader(http.StatusNotFound) } })) defer server.Close() client := newTestClient(t, server) + client.userToken = false + changes := []Change{ - {hash: "h1", message: "First", entries: map[string]FileEntry{"a.txt": newFileEntry([]byte("a"))}}, - {hash: "h2", message: "Second", entries: map[string]FileEntry{"b.txt": newFileEntry([]byte("b"))}}, - {hash: "h3", message: "Third", entries: map[string]FileEntry{"c.txt": newFileEntry([]byte("c"))}}, + {hash: "h1", message: "Exec file", entries: map[string]FileEntry{"script.sh": {Content: []byte("#!/bin/bash"), Mode: "100755"}}}, } - count, sha, err := client.PushChanges(context.Background(), "initial", changes...) + count, _, err := client.PushChanges(context.Background(), "initial", changes...) if err != nil { t.Fatalf("unexpected error: %v", err) } - if count != 3 { - t.Errorf("expected count 3, got %d", count) + if count != 1 { + t.Errorf("expected count 1, got %d", count) } - if sha == "" { - t.Error("expected non-empty sha") + + // Should have updated both the throwaway branch (to sync) and the real branch + if len(updatedRefs) != 2 { + t.Fatalf("expected 2 updated refs, got %d: %v", len(updatedRefs), updatedRefs) + } + + // First update should be the throwaway branch sync + if !strings.Contains(updatedRefs[0], "--headless-tmp-") { + t.Errorf("expected first ref update to be throwaway branch, got %q", updatedRefs[0]) } - if commitCount != 3 { - t.Errorf("expected 3 commits to be created, got %d", commitCount) + // Second update should be the real branch + if updatedRefs[1] != "heads/test-branch" { + t.Errorf("expected second ref update to be real branch, got %q", updatedRefs[1]) } }) - t.Run("failure on second change", func(t *testing.T) { - commitCount := 0 + t.Run("failure on commit does not update real branch", func(t *testing.T) { + var updatedRealBranch bool + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.Header().Set("Content-Type", "application/json") switch { - case strings.HasPrefix(r.URL.Path, "/repos/test-owner/test-repo/git/commits/"): - json.NewEncoder(w).Encode(github.Commit{ - Tree: &github.Tree{SHA: github.Ptr("tree-sha")}, - }) - case r.URL.Path == "/repos/test-owner/test-repo/git/blobs": - w.WriteHeader(http.StatusCreated) - json.NewEncoder(w).Encode(github.Blob{SHA: github.Ptr("blob-sha")}) - case r.URL.Path == "/repos/test-owner/test-repo/git/trees": - w.WriteHeader(http.StatusCreated) - json.NewEncoder(w).Encode(github.Tree{SHA: github.Ptr("tree-sha")}) - case r.Method == http.MethodPost && r.URL.Path == "/repos/test-owner/test-repo/git/commits": - commitCount++ - if commitCount == 2 { - w.WriteHeader(http.StatusInternalServerError) - return - } + case r.Method == http.MethodPost && r.URL.Path == "/repos/test-owner/test-repo/git/refs": w.WriteHeader(http.StatusCreated) - json.NewEncoder(w).Encode(github.Commit{SHA: github.Ptr("commit-sha")}) - case r.Method == http.MethodPatch: + json.NewEncoder(w).Encode(github.Reference{ + Object: &github.GitObject{SHA: github.Ptr("initial")}, + }) + + case r.Method == http.MethodPatch && strings.Contains(r.URL.Path, "heads/test-branch"): + updatedRealBranch = true json.NewEncoder(w).Encode(github.Reference{ Object: &github.GitObject{SHA: github.Ptr("sha")}, }) + + case r.Method == http.MethodDelete: + w.WriteHeader(http.StatusNoContent) + + default: + w.WriteHeader(http.StatusNotFound) } })) defer server.Close() + commitCount := 0 client := newTestClient(t, server) + client.graphql = &mockGraphQL{ + handler: func(query string, variables map[string]any) (json.RawMessage, error) { + commitCount++ + if commitCount == 2 { + return nil, fmt.Errorf("simulated failure") + } + return signedGraphQLResponse("commit-sha"), nil + }, + } + changes := []Change{ {hash: "h1", message: "First", entries: map[string]FileEntry{"a.txt": newFileEntry([]byte("a"))}}, {hash: "h2", message: "Second", entries: map[string]FileEntry{"b.txt": newFileEntry([]byte("b"))}}, - {hash: "h3", message: "Third", entries: map[string]FileEntry{"c.txt": newFileEntry([]byte("c"))}}, } count, _, err := client.PushChanges(context.Background(), "initial", changes...) @@ -658,6 +674,9 @@ func TestPushChanges(t *testing.T) { if count != 2 { t.Errorf("expected count 2 (failed on second), got %d", count) } + if updatedRealBranch { + t.Error("real branch should not have been updated on failure") + } }) t.Run("update ref fails", func(t *testing.T) { @@ -665,26 +684,31 @@ func TestPushChanges(t *testing.T) { w.Header().Set("Content-Type", "application/json") switch { - case strings.HasPrefix(r.URL.Path, "/repos/test-owner/test-repo/git/commits/"): - json.NewEncoder(w).Encode(github.Commit{ - Tree: &github.Tree{SHA: github.Ptr("tree-sha")}, - }) - case r.URL.Path == "/repos/test-owner/test-repo/git/blobs": - w.WriteHeader(http.StatusCreated) - json.NewEncoder(w).Encode(github.Blob{SHA: github.Ptr("blob-sha")}) - case r.URL.Path == "/repos/test-owner/test-repo/git/trees": - w.WriteHeader(http.StatusCreated) - json.NewEncoder(w).Encode(github.Tree{SHA: github.Ptr("tree-sha")}) - case r.Method == http.MethodPost && r.URL.Path == "/repos/test-owner/test-repo/git/commits": + case r.Method == http.MethodPost && r.URL.Path == "/repos/test-owner/test-repo/git/refs": w.WriteHeader(http.StatusCreated) - json.NewEncoder(w).Encode(github.Commit{SHA: github.Ptr("commit-sha")}) + json.NewEncoder(w).Encode(github.Reference{ + Object: &github.GitObject{SHA: github.Ptr("initial")}, + }) + case r.Method == http.MethodPatch: w.WriteHeader(http.StatusConflict) + + case r.Method == http.MethodDelete: + w.WriteHeader(http.StatusNoContent) + + default: + w.WriteHeader(http.StatusNotFound) } })) defer server.Close() client := newTestClient(t, server) + client.graphql = &mockGraphQL{ + handler: func(query string, variables map[string]any) (json.RawMessage, error) { + return signedGraphQLResponse("commit-sha"), nil + }, + } + changes := []Change{ {hash: "h1", message: "First", entries: map[string]FileEntry{"a.txt": newFileEntry([]byte("a"))}}, } @@ -697,6 +721,50 @@ func TestPushChanges(t *testing.T) { t.Errorf("expected 'update ref' error, got %q", err.Error()) } }) + + t.Run("dry run skips throwaway branch", func(t *testing.T) { + client := &Client{ + owner: "test-owner", + repo: "test-repo", + branch: "test-branch", + dryrun: true, + } + + changes := []Change{ + {hash: "h1", message: "First", entries: map[string]FileEntry{"a.txt": newFileEntry([]byte("a"))}}, + } + + count, _, err := client.PushChanges(context.Background(), "initial", changes...) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if count != 1 { + t.Errorf("expected count 1, got %d", count) + } + }) +} + +func TestChooseStrategy(t *testing.T) { + tests := []struct { + name string + userToken bool + mode string + want commitStrategy + }{ + {"default mode any token", false, "100644", strategyGraphQL}, + {"empty mode any token", false, "", strategyGraphQL}, + {"executable with user token", true, "100755", strategyGraphQL}, + {"executable with non-user token", false, "100755", strategyREST}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + client := &Client{userToken: tt.userToken} + change := Change{entries: map[string]FileEntry{"f": {Content: []byte("x"), Mode: tt.mode}}} + if got := client.chooseStrategy(change); got != tt.want { + t.Errorf("chooseStrategy() = %v, want %v", got, tt.want) + } + }) + } } func TestURLHelpers(t *testing.T) { diff --git a/token.go b/token.go index ac16273..75f1b3e 100644 --- a/token.go +++ b/token.go @@ -1,5 +1,17 @@ package main +// isUserToken returns true if the token appears to be a user-associated token +// (PAT, OAuth, or user-to-server) based on its prefix. These tokens produce +// signed commits when used with the GraphQL API. +func isUserToken(token string) bool { + for _, prefix := range []string{"ghp_", "gho_", "ghu_", "github_pat_"} { + if len(token) > len(prefix) && token[:len(prefix)] == prefix { + return true + } + } + return false +} + type envGetter func(string) string func getToken(getter envGetter) string { diff --git a/token_test.go b/token_test.go new file mode 100644 index 0000000..650578c --- /dev/null +++ b/token_test.go @@ -0,0 +1,27 @@ +package main + +import "testing" + +func TestIsUserToken(t *testing.T) { + tests := []struct { + token string + want bool + }{ + {"ghp_abc123", true}, + {"gho_abc123", true}, + {"ghu_abc123", true}, + {"github_pat_abc123", true}, + {"ghs_abc123", false}, + {"ghr_abc123", false}, + {"some-random-token", false}, + {"", false}, + {"ghp_", false}, // prefix only, no actual token content + } + for _, tt := range tests { + t.Run(tt.token, func(t *testing.T) { + if got := isUserToken(tt.token); got != tt.want { + t.Errorf("isUserToken(%q) = %v, want %v", tt.token, got, tt.want) + } + }) + } +} diff --git a/version.go b/version.go index 15e0e75..0e22c18 100644 --- a/version.go +++ b/version.go @@ -1,3 +1,3 @@ package main -const VERSION = "3.0.0" +const VERSION = "3.1.0" From d99d829f922ea311059a56979513df4291e94f0e Mon Sep 17 00:00:00 2001 From: avidal Date: Mon, 30 Mar 2026 12:08:03 -0500 Subject: [PATCH 2/3] fixup! feat: use GraphQL when possible, fall back to REST if necessary --- .github/workflows/test.yml | 12 +++++----- action-template/action.yml | 2 +- github.go | 45 ++++++++++++++++++++++++++++---------- github_test.go | 12 +++++----- token.go | 4 +++- 5 files changed, 50 insertions(+), 25 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 124ff2a..0cc4c33 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -14,9 +14,9 @@ jobs: unit-tests: runs-on: ubuntu-latest steps: - - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 + - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 - - uses: actions/setup-go@d35c59abb061a4a6fb18e82ac0862c26744d6ab5 # v5.5.0 + - uses: actions/setup-go@4a3601121dd01d1626a1e23e37211e3254c1c06c # v6.4.0 with: go-version: '1.24' @@ -27,9 +27,9 @@ jobs: runs-on: ubuntu-latest needs: unit-tests steps: - - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 + - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 - - uses: actions/setup-go@d35c59abb061a4a6fb18e82ac0862c26744d6ab5 # v5.5.0 + - uses: actions/setup-go@4a3601121dd01d1626a1e23e37211e3254c1c06c # v6.4.0 with: go-version: '1.24' @@ -51,7 +51,7 @@ jobs: TEST_BRANCH: test/ci-${{ github.run_id }}-${{ github.job }} steps: - &checkout - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 + uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 with: fetch-depth: 0 @@ -166,7 +166,7 @@ jobs: - &verify-no-tmp-branches name: Verify no leftover working branches run: | - tmp_branches=$(git ls-remote --heads origin | grep -- '--headless-tmp-' || true) + tmp_branches=$(git ls-remote --heads origin | grep -- '${{ env.TEST_BRANCH }}--headless-tmp-' || true) if [ -n "$tmp_branches" ]; then echo "ERROR: leftover working branches found:" echo "$tmp_branches" diff --git a/action-template/action.yml b/action-template/action.yml index 739d1f5..a18a316 100644 --- a/action-template/action.yml +++ b/action-template/action.yml @@ -58,5 +58,5 @@ outputs: description: 'Commit hash of the last commit created' runs: - using: 'node20' + using: 'node24' main: 'action.js' diff --git a/github.go b/github.go index 604de30..5b3e281 100644 --- a/github.go +++ b/github.go @@ -67,6 +67,10 @@ func (g *graphQLClient) Do(ctx context.Context, query string, variables map[stri } defer resp.Body.Close() + if resp.StatusCode != http.StatusOK { + return nil, fmt.Errorf("graphql request: http %d", resp.StatusCode) + } + var result struct { Data json.RawMessage `json:"data"` Errors []struct { @@ -193,7 +197,7 @@ func (c *Client) chooseStrategy(change Change) commitStrategy { func (c *Client) PushChanges(ctx context.Context, headCommit string, changes ...Change) (int, string, error) { if c.dryrun { for i, change := range changes { - _, err := c.CreateChange(ctx, "", headCommit, change) + _, _, err := c.CreateChange(ctx, "", headCommit, change) if err != nil { return i + 1, "", fmt.Errorf("push change %d: %w", i, err) } @@ -224,12 +228,11 @@ func (c *Client) PushChanges(ctx context.Context, headCommit string, changes ... // Create commits on the throwaway branch for i, change := range changes { - newHead, err := c.CreateChange(ctx, tmpBranch, headCommit, change) + newHead, strategy, err := c.CreateChange(ctx, tmpBranch, headCommit, change) if err != nil { return i + 1, "", fmt.Errorf("push change %d: %w", i, err) } - strategy := c.chooseStrategy(change) if strategy == strategyREST { // REST creates a detached commit; advance the throwaway branch to stay in sync _, _, err = c.git.UpdateRef(ctx, c.owner, c.repo, tmpRef, github.UpdateRef{ @@ -262,7 +265,9 @@ func (c *Client) PushChanges(ctx context.Context, headCommit string, changes ... // // tmpBranch is the name of the throwaway branch (without refs/heads/ prefix) used for GraphQL // commits. It may be empty for dry-run mode. -func (c *Client) CreateChange(ctx context.Context, tmpBranch, headCommit string, change Change) (string, error) { +// +// Returns the new commit SHA, the strategy used, and any error. +func (c *Client) CreateChange(ctx context.Context, tmpBranch, headCommit string, change Change) (string, commitStrategy, error) { shortHash := change.hash if len(shortHash) > 8 { shortHash = shortHash[:8] @@ -295,54 +300,72 @@ func (c *Client) CreateChange(ctx context.Context, tmpBranch, headCommit string, if c.dryrun { logger.Notice("Dry run enabled, not writing commit") - return strings.Repeat("0", len(change.hash)), nil + return strings.Repeat("0", len(change.hash)), strategy, nil } - // Build the commit function based on strategy + // Build the commit and reset functions based on strategy. + // resetFn rewinds the throwaway branch before a GraphQL retry so expectedHeadOid matches again. var commitFn func() (string, bool, error) + var resetFn func() error switch strategy { case strategyGraphQL: + tmpRef := fmt.Sprintf("refs/heads/%s", tmpBranch) commitFn = func() (string, bool, error) { return c.execGraphQLCommit(ctx, tmpBranch, headCommit, change) } + resetFn = func() error { + _, _, err := c.git.UpdateRef(ctx, c.owner, c.repo, tmpRef, github.UpdateRef{ + SHA: headCommit, + Force: github.Ptr(true), + }) + if err != nil { + return fmt.Errorf("reset working branch for retry: %w", err) + } + return nil + } case strategyREST: // Prep tree once before the retry loop treeSHA, err := c.prepTree(ctx, headCommit, change) if err != nil { - return "", err + return "", strategy, err } commitFn = func() (string, bool, error) { return c.execRESTCommit(ctx, headCommit, treeSHA, change) } + resetFn = func() error { return nil } } // Execute with signature verification retry commitSha, verified, err := commitFn() if err != nil { - return "", fmt.Errorf("create commit: %w", err) + return "", strategy, fmt.Errorf("create commit: %w", err) } if c.signAttempts > 0 { backoff := 1 * time.Second for attempt := 1; attempt <= c.signAttempts && !verified; attempt++ { if attempt == c.signAttempts { - return "", fmt.Errorf("commit %s was not signed after %d attempt(s)", commitSha, c.signAttempts) + return "", strategy, fmt.Errorf("commit %s was not signed after %d attempt(s)", commitSha, c.signAttempts) } logger.Warningf("Commit %s not signed (attempt %d/%d), retrying in %s...", commitSha, attempt, c.signAttempts, backoff) time.Sleep(backoff) backoff *= 2 + if err := resetFn(); err != nil { + return "", strategy, err + } + commitSha, verified, err = commitFn() if err != nil { - return "", fmt.Errorf("create commit (attempt %d): %w", attempt+1, err) + return "", strategy, fmt.Errorf("create commit (attempt %d): %w", attempt+1, err) } } } logger.Printf("Created: %s\n", c.commitURL(commitSha)) - return commitSha, nil + return commitSha, strategy, nil } // prepTree creates blobs and a tree for the REST commit path. This is called once before the diff --git a/github_test.go b/github_test.go index dc9c50a..4fb631f 100644 --- a/github_test.go +++ b/github_test.go @@ -225,7 +225,7 @@ func TestCreateChange(t *testing.T) { }, } - sha, err := client.CreateChange(context.Background(), "", "head-sha", change) + sha, _, err := client.CreateChange(context.Background(), "", "head-sha", change) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -264,7 +264,7 @@ func TestCreateChange(t *testing.T) { }, } - sha, err := client.CreateChange(context.Background(), "tmp-branch", "parent-sha", change) + sha, _, err := client.CreateChange(context.Background(), "tmp-branch", "parent-sha", change) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -336,7 +336,7 @@ func TestCreateChange(t *testing.T) { }, } - sha, err := client.CreateChange(context.Background(), "tmp-branch", "parent-sha", change) + sha, _, err := client.CreateChange(context.Background(), "tmp-branch", "parent-sha", change) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -379,7 +379,7 @@ func TestCreateChange(t *testing.T) { }, } - sha, err := client.CreateChange(context.Background(), "tmp-branch", "parent-sha", change) + sha, _, err := client.CreateChange(context.Background(), "tmp-branch", "parent-sha", change) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -414,7 +414,7 @@ func TestCreateChange(t *testing.T) { entries: map[string]FileEntry{"file.txt": newFileEntry([]byte("x"))}, } - _, err := client.CreateChange(context.Background(), "tmp", "parent", change) + _, _, err := client.CreateChange(context.Background(), "tmp", "parent", change) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -441,7 +441,7 @@ func TestCreateChange(t *testing.T) { entries: map[string]FileEntry{"script.sh": {Content: []byte("x"), Mode: "100755"}}, } - _, err := client.CreateChange(context.Background(), "tmp", "nonexistent", change) + _, _, err := client.CreateChange(context.Background(), "tmp", "nonexistent", change) if err == nil { t.Fatal("expected error, got nil") } diff --git a/token.go b/token.go index 75f1b3e..927646e 100644 --- a/token.go +++ b/token.go @@ -1,11 +1,13 @@ package main +import "strings" + // isUserToken returns true if the token appears to be a user-associated token // (PAT, OAuth, or user-to-server) based on its prefix. These tokens produce // signed commits when used with the GraphQL API. func isUserToken(token string) bool { for _, prefix := range []string{"ghp_", "gho_", "ghu_", "github_pat_"} { - if len(token) > len(prefix) && token[:len(prefix)] == prefix { + if strings.HasPrefix(token, prefix) && len(token) > len(prefix) { return true } } From fe98aadcebe00515a3ebde53dfd3f327cb8ada05 Mon Sep 17 00:00:00 2001 From: avidal Date: Mon, 30 Mar 2026 12:15:34 -0500 Subject: [PATCH 3/3] fixup! feat: use GraphQL when possible, fall back to REST if necessary --- .github/workflows/release.yml | 29 ++++++++++------------------- 1 file changed, 10 insertions(+), 19 deletions(-) diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 884782c..adc58e3 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -14,8 +14,8 @@ jobs: runs-on: ubuntu-latest steps: - - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 - - uses: actions/setup-go@d35c59abb061a4a6fb18e82ac0862c26744d6ab5 # v5.5.0 + - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + - uses: actions/setup-go@4a3601121dd01d1626a1e23e37211e3254c1c06c # v6.4.0 with: go-version: '1.24' @@ -46,7 +46,7 @@ jobs: binary: commit-headless-linux-arm64 steps: - - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 + - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 - name: download artifacts uses: actions/download-artifact@d3f86a106a0bac45b974a628896c90dbdf5c8093 # v4.3.0 @@ -64,7 +64,7 @@ jobs: runs-on: ubuntu-latest steps: - - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 + - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 with: fetch-depth: 0 # needed to make sure we get all tags @@ -127,23 +127,14 @@ jobs: branch: action command: push - - name: check release tag - id: check-tag + - name: print release instructions run: | TAG="action/v$(cat ./dist/VERSION.txt)" + SHA="${{ steps.push-commits.outputs.pushed_ref }}" + if git show-ref --tags --verify --quiet "refs/tags/${TAG}"; then - echo "Release tag ${TAG} already exists. Not releasing." - exit 1 + echo "::notice::Release tag ${TAG} already exists. Nothing to do." + exit 0 fi - echo "tag=${TAG}" >> $GITHUB_OUTPUT - - name: make release - uses: actions/github-script@60a0d83039c74a4aee543508d2ffcb1c3799cdea # v7.0.1 - with: - script: | - github.rest.git.createRef({ - owner: context.repo.owner, - repo: context.repo.repo, - ref: 'refs/tags/${{ steps.check-tag.outputs.tag }}', - sha: '${{ steps.push-commits.outputs.pushed_ref }}' - }); + echo "::notice::To release, run: git tag ${TAG} ${SHA} && git push origin ${TAG}"