diff --git a/internal/hotfix/finalize.go b/internal/hotfix/finalize.go index b052553e..31a260de 100644 --- a/internal/hotfix/finalize.go +++ b/internal/hotfix/finalize.go @@ -145,6 +145,7 @@ func (gitStatePusher) CommitAndPush(path, branch, message string) error { // whatever trunk bytes the loop fetches. type apiStatePusher struct { mutate statewrite.Mutate + author statewrite.Identity } func (p apiStatePusher) CommitAndPush(path, branch, message string) error { @@ -159,9 +160,21 @@ func (p apiStatePusher) CommitAndPush(path, branch, message string) error { Ref: branch, Message: message, Mutate: p.mutate, + Author: p.author, }) } +// gitIdentity resolves the author/committer for a Contents API state commit from +// the manifest git config, defaulting to the github-actions[bot] identity when +// the config is absent. This attributes the automated state commit to the bot +// rather than the token owner GitHub would otherwise stamp. +func gitIdentity(cfg *config.TrunkConfig) statewrite.Identity { + if cfg == nil { + return statewrite.Identity{} + } + return statewrite.Identity{Name: cfg.GetGitUserName(), Email: cfg.GetGitUserEmail()} +} + // isRealGitHub reports whether the workflow runs on github.com rather than an // act/gitea e2e environment, detected by GITHUB_SERVER_URL as the generated // dispatch steps do. @@ -462,7 +475,7 @@ func (f *Finalizer) Finalize(targetEnv, mergeSHA string, fixSHAs []string, baseS capturedTarget := targetEnv capturedMerge := mergeSHA key := f.manifestKey - pusher = apiStatePusher{mutate: func(current []byte) ([]byte, error) { + pusher = apiStatePusher{author: gitIdentity(f.cicd.Config), mutate: func(current []byte) ([]byte, error) { fresh, err := config.ParseManifestBytes(current, key) if err != nil { return nil, fmt.Errorf("parsing current manifest: %w", err) diff --git a/internal/promote/finalize.go b/internal/promote/finalize.go index 42150e8a..7831c286 100644 --- a/internal/promote/finalize.go +++ b/internal/promote/finalize.go @@ -381,6 +381,17 @@ func isRealGitHub() bool { // loop fetches and overlays only this finalizer's owned env state, so two // concurrent env finalizers merge rather than clobber each other on the file // blob SHA. +// gitIdentity resolves the author/committer for a Contents API state commit from +// the manifest git config, defaulting to the github-actions[bot] identity when +// the config is absent. This attributes the automated state commit to the bot +// rather than the token owner GitHub would otherwise stamp. +func gitIdentity(cfg *config.TrunkConfig) statewrite.Identity { + if cfg == nil { + return statewrite.Identity{} + } + return statewrite.Identity{Name: cfg.GetGitUserName(), Email: cfg.GetGitUserEmail()} +} + func (f *Finalizer) writeStateViaAPI(message string) error { repo := os.Getenv("GITHUB_REPOSITORY") if repo == "" { @@ -397,6 +408,7 @@ func (f *Finalizer) writeStateViaAPI(message string) error { Path: f.configPath, Ref: branch, Message: message, + Author: gitIdentity(f.cicdFile.Config), Mutate: func(current []byte) ([]byte, error) { into, err := config.ParseManifestBytes(current, key) if err != nil { diff --git a/internal/statewrite/apiwrite.go b/internal/statewrite/apiwrite.go index 1d1bdd78..c6b17ff7 100644 --- a/internal/statewrite/apiwrite.go +++ b/internal/statewrite/apiwrite.go @@ -26,6 +26,39 @@ const maxAttempts = 5 // waits N*retryBackoff so concurrent writers stagger rather than re-collide. const retryBackoff = 500 * time.Millisecond +// defaultBotName and defaultBotEmail are the identity stamped on a state commit +// when the manifest git config supplies no override. They match the identity the +// git-based state writers use, so a Contents API commit is attributed to the +// automation bot rather than the token owner. +const ( + defaultBotName = "github-actions[bot]" + defaultBotEmail = "github-actions[bot]@users.noreply.github.com" +) + +// Identity is the name and email stamped as both the author and the committer of +// a Contents API state commit. State writers populate it from the manifest git +// config (GetGitUserName/GetGitUserEmail) so automated commits are attributed to +// the bot identity rather than the token owner that GitHub would otherwise use. +type Identity struct { + // Name is the commit author/committer name. Empty falls back to the bot default. + Name string + // Email is the commit author/committer email. Empty falls back to the bot default. + Email string +} + +// orDefault returns the identity with any empty field filled from the bot +// default, so a commit is always attributed to a concrete identity and behavior +// is never worse than before this attribution was threaded through. +func (id Identity) orDefault() Identity { + if id.Name == "" { + id.Name = defaultBotName + } + if id.Email == "" { + id.Email = defaultBotEmail + } + return id +} + // ContentsClient is the minimal GitHub Contents API surface the retry loop // needs. The production implementation shells out to the gh CLI; tests inject a // fake that returns a 409 on the first PUT and succeeds on the second. @@ -37,7 +70,7 @@ const retryBackoff = 500 * time.Millisecond // when the blob SHA no longer matches. type ContentsClient interface { GetContent(repo, path, ref string) (content []byte, sha string, err error) - PutContent(repo, path, ref, sha, message string, content []byte) error + PutContent(repo, path, ref, sha, message string, content []byte, author Identity) error } // ConflictError reports an optimistic-lock (HTTP 409) failure from the Contents @@ -114,6 +147,11 @@ type Options struct { // Mutate derives the bytes to write from the current manifest bytes. It is // re-applied on every retry. Required. Mutate Mutate + // Author is the identity stamped as both author and committer of the state + // commit. Callers populate it from the manifest git config so the commit is + // attributed to the bot identity. An empty field falls back to the + // github-actions[bot] default. + Author Identity // Sleep is called between retries. Defaults to time.Sleep; tests inject a // no-op so no real time passes. Sleep func(time.Duration) @@ -136,6 +174,7 @@ func CommitWithRetry(opts Options) error { if sleep == nil { sleep = time.Sleep } + author := opts.Author.orDefault() var lastErr error for attempt := 1; attempt <= maxAttempts; attempt++ { @@ -149,7 +188,7 @@ func CommitWithRetry(opts Options) error { return fmt.Errorf("applying state mutation: %w", err) } - err = opts.Client.PutContent(opts.Repo, opts.Path, opts.Ref, sha, opts.Message, next) + err = opts.Client.PutContent(opts.Repo, opts.Path, opts.Ref, sha, opts.Message, next, author) if err == nil { return nil } diff --git a/internal/statewrite/apiwrite_test.go b/internal/statewrite/apiwrite_test.go index 4df5d0ca..06216e75 100644 --- a/internal/statewrite/apiwrite_test.go +++ b/internal/statewrite/apiwrite_test.go @@ -23,10 +23,11 @@ type fakeContents struct { // past the slice default to applying successfully. putErrs []error - puts int // number of PutContent calls - gets int // number of GetContent calls - putSeen []string // content bytes presented to each PutContent - shaSeen []string // sha presented to each PutContent + puts int // number of PutContent calls + gets int // number of GetContent calls + putSeen []string // content bytes presented to each PutContent + shaSeen []string // sha presented to each PutContent + idSeen []Identity // author/committer identity presented to each PutContent } func (f *fakeContents) GetContent(_, _, _ string) ([]byte, string, error) { @@ -34,10 +35,11 @@ func (f *fakeContents) GetContent(_, _, _ string) ([]byte, string, error) { return []byte(f.content), f.sha, nil } -func (f *fakeContents) PutContent(_, _, _, sha, _ string, content []byte) error { +func (f *fakeContents) PutContent(_, _, _, sha, _ string, content []byte, author Identity) error { f.puts++ f.putSeen = append(f.putSeen, string(content)) f.shaSeen = append(f.shaSeen, sha) + f.idSeen = append(f.idSeen, author) if f.puts-1 < len(f.putErrs) { if err := f.putErrs[f.puts-1]; err != nil { return err @@ -167,6 +169,85 @@ func TestCommitWithRetry_NonConflictErrorIsNotRetried(t *testing.T) { assert.Equal(t, 0, slept, "a non-409 error must not back off") } +func TestCommitWithRetry_StampsBotAuthorAndCommitter(t *testing.T) { + // State commits must be attributed to the bot identity (both author and + // committer) so GitHub does not stamp them with the token owner. With no + // override the identity defaults to github-actions[bot]; a manifest git + // override flows through verbatim. + tests := []struct { + name string + give Identity + want Identity + }{ + { + name: "defaults to github-actions[bot] when unset", + give: Identity{}, + want: Identity{ + Name: "github-actions[bot]", + Email: "github-actions[bot]@users.noreply.github.com", + }, + }, + { + name: "honors a manifest git override", + give: Identity{Name: "release-bot", Email: "release-bot@example.com"}, + want: Identity{Name: "release-bot", Email: "release-bot@example.com"}, + }, + { + name: "fills only the missing field from the default", + give: Identity{Name: "release-bot"}, + want: Identity{Name: "release-bot", Email: "github-actions[bot]@users.noreply.github.com"}, + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + fake := &fakeContents{content: "ci.state.test: A\n", sha: "sha-0"} + + err := CommitWithRetry(Options{ + Client: fake, + Repo: "owner/name", + Path: ".github/manifest.yaml", + Ref: "main", + Message: "chore: update state [skip ci]", + Mutate: appendLine("ci.state.staging: B"), + Author: tc.give, + Sleep: noSleep(new(int)), + }) + + require.NoError(t, err) + require.Len(t, fake.idSeen, 1, "exactly one PUT must be issued") + assert.Equal(t, tc.want, fake.idSeen[0], "the PUT must carry the resolved bot identity") + }) + } +} + +func TestCommitWithRetry_IdentityPersistsAcrossRetries(t *testing.T) { + // A 409 retry re-fetches and re-PUTs; the resolved identity must accompany + // every attempt, not just the first. + fake := &fakeContents{ + content: "ci.state.test: A\n", + sha: "sha-0", + putErrs: []error{rawConflict()}, + } + + err := CommitWithRetry(Options{ + Client: fake, + Repo: "owner/name", + Path: ".github/manifest.yaml", + Ref: "main", + Message: "chore: update state [skip ci]", + Mutate: appendLine("ci.state.staging: B"), + Author: Identity{Name: "release-bot", Email: "release-bot@example.com"}, + Sleep: noSleep(new(int)), + }) + + require.NoError(t, err) + require.Len(t, fake.idSeen, 2, "the 409 must drive a second PUT") + for i, got := range fake.idSeen { + assert.Equal(t, Identity{Name: "release-bot", Email: "release-bot@example.com"}, got, + "attempt %d must carry the configured identity", i+1) + } +} + func TestIsConflict(t *testing.T) { tests := []struct { name string diff --git a/internal/statewrite/ghclient.go b/internal/statewrite/ghclient.go index 24700332..56ed5572 100644 --- a/internal/statewrite/ghclient.go +++ b/internal/statewrite/ghclient.go @@ -43,15 +43,22 @@ func (ghContents) GetContent(repo, path, ref string) ([]byte, string, error) { // PutContent writes content at ref through the Contents API. When sha is // non-empty the write is an update guarded by that optimistic-lock token; an -// empty sha creates the file. It classifies a 409 optimistic-lock failure as a +// empty sha creates the file. It stamps author with both the commit author and +// committer so the state commit is attributed to the bot identity rather than +// the token owner, and classifies a 409 optimistic-lock failure as a // ConflictError so the retry loop recognizes it. -func (ghContents) PutContent(repo, path, ref, sha, message string, content []byte) error { +func (ghContents) PutContent(repo, path, ref, sha, message string, content []byte, author Identity) error { + author = author.orDefault() b64 := base64.StdEncoding.EncodeToString(content) args := []string{ "api", fmt.Sprintf("repos/%s/contents/%s", repo, path), "-X", "PUT", "-f", "message=" + message, "-f", "content=" + b64, "-f", "branch=" + ref, + "-f", "author[name]=" + author.Name, + "-f", "author[email]=" + author.Email, + "-f", "committer[name]=" + author.Name, + "-f", "committer[email]=" + author.Email, } if sha != "" { args = append(args, "-f", "sha="+sha)