From 48ebe446a46fa8f02f5c9dba28cd159302592857 Mon Sep 17 00:00:00 2001 From: mintaka Date: Tue, 8 Sep 2026 15:31:34 -0400 Subject: [PATCH 1/2] feat(github): carry the push event's previous head on the pipeline The config extension needs the base of a push range to compute which files changed across a multi-commit push. GitHub sends this as the `before` field on the push hook, and `parsePushHook` already returns it, but `Hook` used it only as an argument to `CompareCommits` inside `loadChangedFilesFromCommits` and then discarded it. Anything downstream of the pipeline had no way to see it, so a consumer had to fall back to a `~1` floor and miss files on a push of more than one commit. Assign it to a new `Pipeline.Before` field. `server/services/config` serializes the pipeline wholesale, so the value reaches a config extension with no change to the extension API. Guard the assignment with `usablePushBase` rather than storing the raw value. `loadChangedFilesFromCommits` normalizes an all-zero SHA and a `prev == curr` self-compare internally without mutating its caller, so those unusable values would otherwise reach the payload and a consumer would have to re-derive the same rules. The all-zero literal now lives in one `zeroSHA` const shared by both sites so the two cannot drift. The column is persisted, not `xorm:"-"`. Restarting a pipeline hands `config.Fetch` a store-loaded row (`restart.go` passes `lastPipeline`, loaded by `PostPipeline`), so an in-memory-only field would read empty on every restart and silently regress a consumer to the `~1` floor at exactly the moment a human retries a failed run. No migration file is needed: `model.Pipeline` is in `allBeans` and `syncAll` runs `sess.Sync`, which adds the column. Scoped to the GitHub forge. Gitea and Forgejo parse and discard an equivalent value; those are left to the upstream discussion. Co-authored-by: Matt Wilkinson --- cmd/server/openapi/docs.go | 4 + server/forge/github/github.go | 15 ++- server/forge/github/push_base_test.go | 168 ++++++++++++++++++++++++++ server/model/pipeline.go | 1 + server/model/pipeline_test.go | 28 +++++ 5 files changed, 215 insertions(+), 1 deletion(-) create mode 100644 server/forge/github/push_base_test.go diff --git a/cmd/server/openapi/docs.go b/cmd/server/openapi/docs.go index d1b04160459..5568810f8d8 100644 --- a/cmd/server/openapi/docs.go +++ b/cmd/server/openapi/docs.go @@ -5076,6 +5076,10 @@ const docTemplate = `{ "author_email": { "type": "string" }, + "before": { + "description": "previous head commit of the pushed range, empty when there is no usable base (force push, tag, new branch)", + "type": "string" + }, "branch": { "type": "string" }, diff --git a/server/forge/github/github.go b/server/forge/github/github.go index d9a35f7005e..1628527b463 100644 --- a/server/forge/github/github.go +++ b/server/forge/github/github.go @@ -55,6 +55,9 @@ const ( // slow GitHub call does not fail with "context deadline exceeded" mid-report. statusReportTimeout = 30 * time.Second githubClientKey contextKey = "github_client" + // The all-zero commit id GitHub reports as the "before" commit of a push + // that creates a ref. It never names a real commit. + zeroSHA = "0000000000000000000000000000000000000000" ) // Opts defines configuration options. @@ -781,6 +784,9 @@ func (c *client) Hook(ctx context.Context, r *http.Request) (*model.Repo, *model return nil, nil, err } } else if pipeline != nil && pipeline.Event == model.EventPush { + if usablePushBase(currCommit, prevCommit) { + pipeline.Before = prevCommit + } // GitHub has removed commit summaries from Events API payloads from 7th October 2025 onwards. pipeline, err = c.loadChangedFilesFromCommits(ctx, repo, pipeline, currCommit, prevCommit) if err != nil { @@ -890,6 +896,13 @@ func (c *client) getTagCommitSHA(ctx context.Context, repo *model.Repo, tagName return tag.GetCommit().GetSHA(), nil } +// usablePushBase reports whether prev names a commit a push can be compared +// against. GitHub sends the all-zero SHA when the push creates the ref, and +// repeats curr when the ref did not move, neither of which is a usable base. +func usablePushBase(curr, prev string) bool { + return prev != "" && prev != zeroSHA && prev != curr +} + func (c *client) loadChangedFilesFromCommits(ctx context.Context, tmpRepo *model.Repo, pipeline *model.Pipeline, curr, prev string) (*model.Pipeline, error) { _store, ok := store.TryFromContext(ctx) if !ok { @@ -901,7 +914,7 @@ func (c *client) loadChangedFilesFromCommits(ctx context.Context, tmpRepo *model case curr: log.Error().Msg("GitHub push event contains the same commit before and after, no changes detected") return pipeline, nil - case "0000000000000000000000000000000000000000": + case zeroSHA: prev = "" fallthrough case "": diff --git a/server/forge/github/push_base_test.go b/server/forge/github/push_base_test.go new file mode 100644 index 00000000000..995ae6029c8 --- /dev/null +++ b/server/forge/github/push_base_test.go @@ -0,0 +1,168 @@ +// Copyright 2026 Woodpecker Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package github + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/google/go-github/v90/github" + github_mock "github.com/migueleliasweb/go-github-mock/src/mock" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" + + "go.woodpecker-ci.org/woodpecker/v3/server/forge/github/fixtures" + "go.woodpecker-ci.org/woodpecker/v3/server/model" + "go.woodpecker-ci.org/woodpecker/v3/server/store" + store_mocks "go.woodpecker-ci.org/woodpecker/v3/server/store/mocks" +) + +func TestUsablePushBase(t *testing.T) { + const curr = "366701fde727cb7a9e7f21eb88264f59f6f9b89c" + + tests := []struct { + name string + prev string + want bool + }{ + {name: "distinct previous head is a usable base", prev: "2f780193b136b72bfea4eeb640786a8c4450c7a2", want: true}, + {name: "all-zero SHA is not a usable base", prev: zeroSHA}, + {name: "previous head equal to current is not a usable base", prev: curr}, + {name: "empty previous head is not a usable base", prev: ""}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + assert.Equal(t, tc.want, usablePushBase(curr, tc.prev)) + }) + } +} + +// jsonHandler serves body as JSON on every request, unlike the single-shot FIFO +// entry WithRequestMatch installs. +func jsonHandler(t *testing.T, body any) http.Handler { + t.Helper() + + return http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + if err := json.NewEncoder(w).Encode(body); err != nil { + t.Errorf("encoding mocked GitHub response: %v", err) + } + }) +} + +// pushHookWithBefore rewrites the "before" commit of the sample push hook, +// leaving the rest of the payload untouched. GitHub varies only that field +// between an ordinary push, a ref creation and a no-op push. +func pushHookWithBefore(t *testing.T, before string) string { + t.Helper() + + var payload map[string]any + require.NoError(t, json.Unmarshal([]byte(fixtures.HookPush), &payload)) + payload["before"] = before + + raw, err := json.Marshal(payload) + require.NoError(t, err) + return string(raw) +} + +// pushBaseTestClient wires a client whose changed-file lookups are mocked and +// whose store resolves to a fake repo and user, mirroring the harness in +// TestHook. Only pipeline.Before is under test here; the changed-file payloads +// exist so Hook can run to completion, so the endpoints are served by handlers +// that answer any number of times rather than the FIFO queue WithRequestMatch +// installs, which a table of subtests would exhaust. +func pushBaseTestClient(t *testing.T) (*client, context.Context) { + t.Helper() + + changedFile := []*github.CommitFile{{Filename: github.Ptr("main.go")}} + mockedHTTPClient := github_mock.NewMockedHTTPClient( + github_mock.WithRequestMatchHandler( + github_mock.GetReposCommitsByOwnerByRepoByRef, + jsonHandler(t, github.RepositoryCommit{Files: changedFile}), + ), + github_mock.WithRequestMatchHandler( + github_mock.GetReposCompareByOwnerByRepoByBasehead, + jsonHandler(t, github.CommitsComparison{Files: changedFile}), + ), + ) + + gh, err := github.NewClient(github.WithHTTPClient(mockedHTTPClient)) + require.NoError(t, err) + + mockStore := store_mocks.NewMockStore(t) + mockStore.On("GetUser", mock.Anything).Return(&model.User{ID: 1, Login: "6543", AccessToken: "token"}, nil).Maybe() + mockStore.On("GetRepoNameFallback", mock.Anything, mock.Anything, mock.Anything).Return(&model.Repo{ + ID: 1, + ForgeRemoteID: "1", + Owner: "6543", + Name: "hello-world", + UserID: 1, + }, nil).Maybe() + + ctx := context.WithValue(t.Context(), githubClientKey, gh) + ctx = store.InjectToContext(ctx, mockStore) + + return &client{API: defaultAPI, url: defaultURL}, ctx +} + +func TestHookPushBefore(t *testing.T) { + const head = "366701fde727cb7a9e7f21eb88264f59f6f9b89c" + + tests := []struct { + name string + payload string + want string + }{ + { + name: "ordinary push carries the previous head", + payload: fixtures.HookPush, + want: "2f780193b136b72bfea4eeb640786a8c4450c7a2", + }, + { + name: "branch creation drops the all-zero base", + payload: pushHookWithBefore(t, zeroSHA), + }, + { + name: "push that does not move the ref drops the base", + payload: pushHookWithBefore(t, head), + }, + { + name: "force push drops the rewritten base", + payload: fixtures.HookPushForced, + }, + } + + c, ctx := pushBaseTestClient(t) + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + req := httptest.NewRequest("POST", "/hook", strings.NewReader(tc.payload)) + req.Header.Set("Content-Type", "application/json") + req.Header.Set("X-GitHub-Event", "push") + + _, pipeline, err := c.Hook(ctx, req) + require.NoError(t, err) + require.NotNil(t, pipeline) + require.Equal(t, model.EventPush, pipeline.Event) + assert.Equal(t, tc.want, pipeline.Before) + }) + } +} diff --git a/server/model/pipeline.go b/server/model/pipeline.go index 48f1c179e42..dd357e337e3 100644 --- a/server/model/pipeline.go +++ b/server/model/pipeline.go @@ -39,6 +39,7 @@ type Pipeline struct { DeployTo string `json:"deploy_to" xorm:"deploy"` DeployTask string `json:"deploy_task" xorm:"deploy_task"` Commit string `json:"commit" xorm:"commit"` + Before string `json:"before,omitempty" xorm:"before"` // previous head commit of the pushed range, empty when there is no usable base (force push, tag, new branch) Branch string `json:"branch" xorm:"branch"` RerunCount int64 `json:"rerun_count" xorm:"rerun_count"` Ref string `json:"ref" xorm:"ref"` diff --git a/server/model/pipeline_test.go b/server/model/pipeline_test.go index e728fbae6ef..df81f2f37c3 100644 --- a/server/model/pipeline_test.go +++ b/server/model/pipeline_test.go @@ -15,9 +15,11 @@ package model import ( + "encoding/json" "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestPipelineToAPIModel(t *testing.T) { @@ -75,3 +77,29 @@ func TestPipelineToAPIModel(t *testing.T) { }) } } + +func TestPipelineBeforeJSON(t *testing.T) { + t.Run("before is carried in the payload when set", func(t *testing.T) { + raw, err := json.Marshal(Pipeline{ + Commit: "366701fde727cb7a9e7f21eb88264f59f6f9b89c", + Before: "2f780193b136b72bfea4eeb640786a8c4450c7a2", + }) + require.NoError(t, err) + + var payload map[string]any + require.NoError(t, json.Unmarshal(raw, &payload)) + assert.Equal(t, "2f780193b136b72bfea4eeb640786a8c4450c7a2", payload["before"]) + }) + + t.Run("before is omitted from the payload when empty", func(t *testing.T) { + raw, err := json.Marshal(Pipeline{Commit: "366701fde727cb7a9e7f21eb88264f59f6f9b89c"}) + require.NoError(t, err) + + var payload map[string]any + require.NoError(t, json.Unmarshal(raw, &payload)) + // commit has no omitempty, so its presence proves the payload really was + // inspected and "before" is absent by the tag, not by a broken decode. + assert.Contains(t, payload, "commit") + assert.NotContains(t, payload, "before") + }) +} From 7d46e7e83905e972be8b2f250dcfec1369c4f4f5 Mon Sep 17 00:00:00 2001 From: mintaka Date: Tue, 8 Sep 2026 16:20:45 -0400 Subject: [PATCH 2/2] fix(github): close the review findings on the push-base field Four fixes from the review of the parent commit. Add a store round-trip test for `Before`. The parent commit argues at length that the field must be a real column rather than `xorm:"-"`, because a restart hands `config.Fetch` a store-loaded row, but nothing tested it: mutating the tag to `xorm:"-"` left the whole server suite green, so the regression it warns about would have shipped silently. The test asserts the value survives `CreatePipeline` and comes back from `GetPipelineNumber`, which is the accessor the restart path uses. Name the forge scope in the field's doc comment. That text is published verbatim into the public OpenAPI spec, and it listed three reasons the value can be empty while omitting the two most likely ones: the field is assigned only in the GitHub forge, so it is always empty on Gitea and Forgejo, and the ref-did-not-move case is rejected by the guard. A consumer reading the old text would reasonably infer a non-empty value is available everywhere. Regenerated the spec to match. Validate both ends of the range in `usablePushBase`. It checked only `prev`, so a push carrying no head commit produced a pipeline with `Before` set and `Commit` empty, a half-open range nothing can compute from. The guard's own comment claimed it reported whether a push could be compared, and a comparison needs both ends. Document the field in the configuration-extension payload example, since delivering it to an extension is the whole point of the change. Co-authored-by: Matt Wilkinson --- cmd/server/openapi/docs.go | 2 +- .../40-configuration-extension.md | 6 ++++ server/forge/github/github.go | 10 +++--- server/forge/github/push_base_test.go | 14 +++++--- server/model/pipeline.go | 2 +- server/store/datastore/pipeline_test.go | 35 +++++++++++++++++++ 6 files changed, 58 insertions(+), 11 deletions(-) diff --git a/cmd/server/openapi/docs.go b/cmd/server/openapi/docs.go index 5568810f8d8..267b46d91d5 100644 --- a/cmd/server/openapi/docs.go +++ b/cmd/server/openapi/docs.go @@ -5077,7 +5077,7 @@ const docTemplate = `{ "type": "string" }, "before": { - "description": "previous head commit of the pushed range, empty when there is no usable base (force push, tag, new branch)", + "description": "previous head commit of the pushed range; GitHub push events only, empty when there is no usable base (force push, tag, new branch, or a ref that did not move)", "type": "string" }, "branch": { diff --git a/docs/docs/20-usage/72-extensions/40-configuration-extension.md b/docs/docs/20-usage/72-extensions/40-configuration-extension.md index f263a0693c8..b532b71c0bc 100644 --- a/docs/docs/20-usage/72-extensions/40-configuration-extension.md +++ b/docs/docs/20-usage/72-extensions/40-configuration-extension.md @@ -102,6 +102,7 @@ Example request: "author": "myUser", "author_avatar": "https://myforge.com/avatars/d6b3f7787a685fcdf2a44e2c685c7e03", "author_email": "my@email.com", + "before": "1a2b3c4d5e6f708192a3b4c5d6e7f8091a2b3c4d", "branch": "main", "changed_files": ["some-filename.txt"], "commit": "2fff90f8d288a4640e90f05049fe30e61a14fd50", @@ -144,6 +145,11 @@ Example request: } ``` +`pipeline.before` holds the previous head of a pushed commit range, which an extension can use +as the base for its own changed-file comparison. It is populated for GitHub push events only, +and is absent when the push has no usable base: a force push, a tag, a new branch, or a ref +that did not move. + ### Response The extension should respond with a JSON payload containing the new configuration files in Woodpecker's official YAML format. diff --git a/server/forge/github/github.go b/server/forge/github/github.go index 1628527b463..421eccca1d1 100644 --- a/server/forge/github/github.go +++ b/server/forge/github/github.go @@ -896,11 +896,13 @@ func (c *client) getTagCommitSHA(ctx context.Context, repo *model.Repo, tagName return tag.GetCommit().GetSHA(), nil } -// usablePushBase reports whether prev names a commit a push can be compared -// against. GitHub sends the all-zero SHA when the push creates the ref, and -// repeats curr when the ref did not move, neither of which is a usable base. +// usablePushBase reports whether prev and curr name commits a push can be +// compared across. GitHub sends the all-zero SHA when the push creates the +// ref, and repeats curr when the ref did not move, neither of which is a +// usable base. A push carrying no head commit leaves curr empty, which is +// not a usable end of the range either. func usablePushBase(curr, prev string) bool { - return prev != "" && prev != zeroSHA && prev != curr + return curr != "" && prev != "" && prev != zeroSHA && prev != curr } func (c *client) loadChangedFilesFromCommits(ctx context.Context, tmpRepo *model.Repo, pipeline *model.Pipeline, curr, prev string) (*model.Pipeline, error) { diff --git a/server/forge/github/push_base_test.go b/server/forge/github/push_base_test.go index 995ae6029c8..066882b7e44 100644 --- a/server/forge/github/push_base_test.go +++ b/server/forge/github/push_base_test.go @@ -37,20 +37,24 @@ import ( func TestUsablePushBase(t *testing.T) { const curr = "366701fde727cb7a9e7f21eb88264f59f6f9b89c" + const prev = "2f780193b136b72bfea4eeb640786a8c4450c7a2" + tests := []struct { name string + curr string prev string want bool }{ - {name: "distinct previous head is a usable base", prev: "2f780193b136b72bfea4eeb640786a8c4450c7a2", want: true}, - {name: "all-zero SHA is not a usable base", prev: zeroSHA}, - {name: "previous head equal to current is not a usable base", prev: curr}, - {name: "empty previous head is not a usable base", prev: ""}, + {name: "distinct previous head is a usable base", curr: curr, prev: prev, want: true}, + {name: "all-zero SHA is not a usable base", curr: curr, prev: zeroSHA}, + {name: "previous head equal to current is not a usable base", curr: curr, prev: curr}, + {name: "empty previous head is not a usable base", curr: curr, prev: ""}, + {name: "empty current head is not a usable base", curr: "", prev: prev}, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { - assert.Equal(t, tc.want, usablePushBase(curr, tc.prev)) + assert.Equal(t, tc.want, usablePushBase(tc.curr, tc.prev)) }) } } diff --git a/server/model/pipeline.go b/server/model/pipeline.go index dd357e337e3..0aa83f54139 100644 --- a/server/model/pipeline.go +++ b/server/model/pipeline.go @@ -39,7 +39,7 @@ type Pipeline struct { DeployTo string `json:"deploy_to" xorm:"deploy"` DeployTask string `json:"deploy_task" xorm:"deploy_task"` Commit string `json:"commit" xorm:"commit"` - Before string `json:"before,omitempty" xorm:"before"` // previous head commit of the pushed range, empty when there is no usable base (force push, tag, new branch) + Before string `json:"before,omitempty" xorm:"before"` // previous head commit of the pushed range; GitHub push events only, empty when there is no usable base (force push, tag, new branch, or a ref that did not move) Branch string `json:"branch" xorm:"branch"` RerunCount int64 `json:"rerun_count" xorm:"rerun_count"` Ref string `json:"ref" xorm:"ref"` diff --git a/server/store/datastore/pipeline_test.go b/server/store/datastore/pipeline_test.go index 6477244d8b5..903fde477af 100644 --- a/server/store/datastore/pipeline_test.go +++ b/server/store/datastore/pipeline_test.go @@ -122,6 +122,41 @@ func TestPipelines(t *testing.T) { assert.EqualValues(t, pipeline2, GetPipeline) } +// TestPipelineBeforePersists pins Before to a real column. The config +// extension reads it off a pipeline the store loaded, which is what a restart +// hands to config.Fetch, so an in-memory-only field would read empty there and +// silently narrow the compared range instead of failing. +func TestPipelineBeforePersists(t *testing.T) { + const before = "2f780193b136b72bfea4eeb640786a8c4450c7a2" + + repo := &model.Repo{ + UserID: 1, + FullName: "bradrydzewski/test", + Owner: "bradrydzewski", + Name: "test", + } + + store, closer := newTestStore(t, new(model.Repo), new(model.Step), new(model.Pipeline)) + defer closer() + + assert.NoError(t, store.CreateRepo(repo)) + + pipeline := model.Pipeline{ + RepoID: repo.ID, + Status: model.StatusSuccess, + Event: model.EventPush, + Branch: "some-branch", + Commit: "366701fde727cb7a9e7f21eb88264f59f6f9b89c", + Before: before, + } + assert.NoError(t, store.CreatePipeline(&pipeline)) + + // GetPipelineNumber is the accessor the restart path uses. + loaded, err := store.GetPipelineNumber(repo, pipeline.Number) + assert.NoError(t, err) + assert.Equal(t, before, loaded.Before) +} + func TestPipelineListFilter(t *testing.T) { repo := &model.Repo{ UserID: 1,