From 529994807fcb2b72c222b7d1ba0f5952bc05c949 Mon Sep 17 00:00:00 2001 From: Joshua Temple Date: Thu, 25 Jun 2026 16:20:07 -0400 Subject: [PATCH] fix(statewrite): attribute state commits to the bot, not the token owner State writes go through the GitHub Contents API, which stamps the commit with the token owner when no author or committer is set, so automated update-state commits carried the maintainer's name across every repo. Thread the configured git identity (GetGitUserName/GetGitUserEmail, defaulting to github-actions[bot] and overridable per repo via the manifest git config) into the PUT as both author and committer. Forward-looking only; existing history is unchanged. Signed-off-by: Joshua Temple --- internal/hotfix/finalize.go | 15 ++++- internal/promote/finalize.go | 12 ++++ internal/statewrite/apiwrite.go | 43 ++++++++++++- internal/statewrite/apiwrite_test.go | 91 ++++++++++++++++++++++++++-- internal/statewrite/ghclient.go | 11 +++- 5 files changed, 162 insertions(+), 10 deletions(-) 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)