Skip to content

feat(github): carry the push event's previous head on the pipeline - #35

Draft
rigel-mintaka wants to merge 2 commits into
mainfrom
upstream-woodpecker-push-before
Draft

feat(github): carry the push event's previous head on the pipeline#35
rigel-mintaka wants to merge 2 commits into
mainfrom
upstream-woodpecker-push-before

Conversation

@rigel-mintaka

@rigel-mintaka rigel-mintaka commented Sep 8, 2026 •
edited
Loading

Copy link
Copy Markdown

Fixes RIG-3539.

What

Adds Pipeline.Before, carrying the previous head of a pushed commit range, and populates it in the GitHub forge's push-hook path.

Why

Our config extension computes the set of changed files for a push so it can decide which pipelines to schedule. For a push of more than one commit it needs the base of the range. GitHub sends exactly that as the push hook's before field, and parsePushHook already returns it, but Hook used it only as an argument to CompareCommits inside loadChangedFilesFromCommits and then dropped it. Nothing downstream of the pipeline could see it, so the extension fell back to a <head>~1 floor and missed files whenever a push carried several commits.

server/services/config serializes the pipeline wholesale into its request, so a json-tagged field on model.Pipeline reaches the extension with no change to the extension API.

Design notes

Three choices worth flagging for review:

The assignment is guarded by usablePushBase, not raw. loadChangedFilesFromCommits normalizes an all-zero SHA (branch or tag creation) and a prev == curr self-compare internally, but it does so without mutating its caller, so those unusable values would otherwise land in the payload and every consumer would have to re-derive the same rules. The all-zero literal now lives in a single zeroSHA const shared by both sites so the two cannot drift apart.

The column is persisted rather than xorm:"-". Restarting a pipeline hands config.Fetch a store-loaded row: restart.go passes lastPipeline, which PostPipeline loads from the store. An in-memory-only field would therefore read empty on every restart and silently regress the extension to the <head>~1 floor at exactly the moment a human retries a failed run. createNewOutOfOld copies the struct, so the value is inherited by the new pipeline.

No migration file is needed. model.Pipeline is in allBeans and syncAll runs sess.Sync(bean), which adds the column. Verified against SQLite; Postgres and MySQL are worth a reviewer's eye, since before is a SQL keyword in some dialects (several sibling columns here inner-quote for that reason, e.g. 'id', 'version', 'timestamp').

Scoped to the GitHub forge. Gitea and Forgejo parse and discard an equivalent value. We run GitHub; the general case is going upstream separately (RIG-3534).

Verification

  • go build ./... clean.
  • go test ./server/forge/github/... ./server/model/... passes.
  • golangci-lint run on the touched packages: 0 issues.
  • gofmt -l clean.
  • make generate-openapi run and the generated hunk committed, so check-openapi sees an empty diff. Regeneration is idempotent (identical file hash on a second run).
  • go test ./server/store/datastore/... -run TestPipeline passes, i.e. xorm creates the column.
  • New tests: TestUsablePushBase (4 cases) and TestHookPushBefore (4 cases, driven end to end through c.Hook), plus TestPipelineBeforeJSON.
  • Both guards mutation-proved: deleting the pipeline.Before assignment fails TestHookPushBefore; dropping the prev != zeroSHA term fails both the all-zero-SHA case and the branch-creation case.

Review round 1 (fixed in 4850b4d4b)

A review of the first commit returned high: 0, medium: 2, low: 2. All four are fixed additively in the second commit so the interdiff is reviewable.

  • M1 — the persistence property had no test. Mutating the tag to xorm:"-" left the entire server suite green, so the regression this PR's own description warns about would have shipped silently. Added a store round-trip through GetPipelineNumber, the accessor the restart path uses. Mutation-proved: the xorm:"-" mutant now fails by name.
  • M2 — the field doc omitted the two most likely reasons the value is empty (GitHub-only assignment, so it is always empty on Gitea and Forgejo; and the ref-did-not-move case). That text is published verbatim into the public OpenAPI spec. Reworded and regenerated.
  • L1 — usablePushBase validated only prev, so a push with no head commit yielded Before set with Commit empty. Made the guard total on both ends and added a covering case. Mutation-proved by name.
  • L2 — documented the field in the configuration-extension payload example.

The reserved-word question raised on round 1 resolved as not a risk, and the recommendation was to change nothing: xorm constructs every dialect quoter with IsReserved: schemas.AlwaysReserve, so identifiers are quoted unconditionally, and sibling columns commit, ref and event are already reserved words shipped with bare tags. xorm:"before" stays as written.

CI status

Two failing workflows on this head, neither caused by this diff:

  • test / sqlite — TestMigrate crossed upstream's shared -timeout 300s. My earlier head 91dbc6a66 passed the same step in 171s; this head took 361s. The one datastore test added here costs 0.01s under -race, and the exact CI invocation passes locally in 62.7s on this tree against 184.9s on a pristine base clone. The runner shares a machine whose load average was ~225 at the time. The timeout value is upstream's (git log -L shows the only local touch is the v2 to v3 path rewrite, directly after upstream's own "increase test timeouts"), so raising it in the fork would mask contention and diverge from upstream.
  • securityscan — Trivy CVEs in golang.org/x/crypto v0.55.0 and a qs npm advisory. This commit touches zero dependency manifests and x/crypto is inherited verbatim from base 0b881dd3a. Pre-existing debt on fork main.

Spec-impact: none. Additive field on an existing model and no change to any request or response contract.

@rigel-mintaka @mattwilkinsonn
feat(github): carry the push event's previous head on the pipeline
91dbc6a 
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 `<head>~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 `<head>~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 <matt@rigel.build>
@linear-code

linear-code Bot commented Sep 8, 2026

Copy link
Copy Markdown

RIG-3539

@rigel-mintaka @mattwilkinsonn
fix(github): close the review findings on the push-base field
4850b4d 
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 <matt@rigel.build>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@rigel-mintaka