vole

Clone

Read only, no credentials:

https://rodent.software/vole.git

Write over SSH with a registered key:

git clone ssh://git@rodent.software:2222/vole.git

A free and open-source forge that lives in your Git repo.

#64

P2: Test the merge result, not just the change-request head, before landing

closed · opened by keenan 12d ago

keenan opened this 12d ago

Why

When a protected branch requires checks, the gate asks for runs of the request head:

// forge/landing/gate.go:111-121
if g.ACL.RequiresChecks() {
    gaps, err := g.CheckGaps(m.Request.Head, branch)
    ...
}

A landing whose base has moved is not that commit. Prepare builds a merge commit (MergeTree + CommitMergeTree, forge/landing/land.go:190-205) and moves the base to it, but CheckGaps still looks up m.Request.Head, and nothing queues a run for the merge commit: server/ci.go only enqueues from pushes. So for any non-fast-forward landing, the branch receives a tree that was never built or tested. Both sides can be green and still break when combined: renamed symbol, changed signature, duplicate registration, migration order, or a test added to the base that the head never saw.

The same lookup also reads workflows at the head (ci.At(g.Repo, sha)), so a workflow change on the base does not apply to the landing.

There is a separate hole in the same path. FindApprovedMatch accepts a pushed merge commit from its parents alone, without checking its tree:

// metadata/requests.go:509-518
if len(parents) == 2 {
    matches = (parents[0] == req.OldTip && parents[1] == r.Head) || ...
}

“Merge, then push” (documented at docs/change-requests.md:128-132) can therefore land content that was neither approved nor tested, as long as the commit has the right two parents. Any fix should close that too.

Fast-forward-only: what it costs

Requiring FF (current tip is an ancestor of head) does close the test gap: the landed tree is the head tree, and the head’s green run covers it. But as the default it is expensive:

Approval churn. Approvals name base + head (metadata/requests.go:529), checks name the head SHA, and contributor CI allow names the exact tip (docs/workflows.md:130-139). Any base move forces every open request to move its head (rebase, or merge the base in), voiding both approvals and checks. Landing N requests costs roughly N(N-1) re-approvals and CI runs; reviewer availability becomes the throughput limit.
Livelock and races. There is no queue or fairness, so under steady traffic a request can be perpetually behind. Two simultaneous landings used to self-heal through the retry in Land (forge/landing/land.go:104-118); with FF-only the loser must go back for a rebase and fresh approval.
Maintainer friction. Non-admin maintainers cannot rewrite their own non-contrib branches (server/hook/hook.go:219-227), so rebasing needs an admin, a delete/recreate, or merging the base into the branch. Contributor tips need a fresh Allow CI per rebase.
Pinned and stacked requests. A bare-commit request (--head <sha>) can never move, and stacked requests need another rebase and head change after the lower one lands.
History. There is no Merge change request <id>: <title> commit; git log --first-parent loses review units, and reverting becomes a range revert. FF-only also does not force linear history unless merge commits in the branch are separately banned.

It could be an opt-in strict mode for a repository that wants linear history, but it is a poor primary fix.

Options

A. Test the candidate merge commit (recommended direction)

Make the thing that lands the thing that was tested:

On demand (merge preflight) or when a request is approved, compute the candidate with the code Prepare already uses: C = CommitMergeTree(MergeTree(baseTip, head), [baseTip, head]).
Publish C under a server-only ref such as refs/vole/candidates/<request-id>/<baseTip>-<head>, and enqueue the applicable jobs against C with Branch = req.Base. The ref is needed: the runner fetches origin <sha> (ci/agent/worktree.go:96-110) and repository creation does not set uploadpack.allowAnySHA1InWant (forge/repo/repo.go), so an unreferenced commit is not fetchable. refs/vole/* is already outside the only writable namespaces (refs/heads/*, refs/tags/*).
Require green required-job results for C, and read workflows at C (the candidate tree includes both sides). For an FF landing C == head, so nothing changes and no duplicate run is needed.
If the base moves, C changes. The old result is stale and a new run is needed, but approvals stay valid because the head did not move. That is the property FF-only loses.

Two delivery shapes:

A1, explicit test-then-merge. vole change test-merge <id> (and a request-page button) publishes and queues the candidate; when it is green, Merge is enabled and the gate checks the candidate for the current base. If the base moved, the button says “stale, test again”. Small surface, no worker, easy to reason about.
A2, landing queue. A worker leases approved requests, computes the candidate, waits for green, lands, and comments on red. Base moves are retried automatically and approvals never churn. More operational machinery: queue state, fairness, timeouts, cancellation. The right end state if traffic is steady.

Keep conflict resolution out of the server. If MergeTree conflicts, or the tree needs manual resolution, stop and require the author to push a new head (which needs fresh approval). A server that resolves conflicts produces a commit nobody reviewed.

B. Serialized merge train (Bors/GitHub merge queue)

Queue approved requests, merge them in order into a candidate train, test the train, land it on green, and retry or bisect on red. Highest throughput under contention, and catches cross-request interactions; also the most machinery. Only worth it if A2’s per-request cost becomes the bottleneck.

C. Verify the landed tree without testing it (partial, do anyway)

For a pushed merge commit, require its tree to equal MergeTree(oldTip, head); reject a two-parent commit that carries extra or different content. This closes the “merge, then push” hole from metadata/requests.go:509-518 and makes the approval attestation mean what it says. It does not test the merge, so it is a hardening step, not the fix. It also means a human-resolved merge cannot ride an old approval; it must become a new head and be re-approved and re-tested.

D. Require up to date / FF-only (rejected as default)

See above. Acceptable as an opt-in branch policy (require_up_to_date, or a linear-history rule) for repositories that value that history more than throughput.

E. Content-addressed approvals (escape hatch for FF-only)

If FF-only is kept, approve a patch fingerprint (merge-base..head diff) rather than a SHA, so a clean rebase does not need re-approval. This weakens “approval names the exact commit”, needs a careful fingerprint definition, and must invalidate on conflict resolution. It reduces the O(N^2) approval cost but not the CI cost or the rebase traps, so it patches D rather than replacing A.

F. Warn only

Show “merge result untested” and keep landing. Rejected: the gate would still report green for a commit other than the one that lands, which is the bug.

Recommendation

Do C now (small, closes a security-relevant hole), then A1 as the minimum fix, growing to A2 if merge traffic makes explicit preflight annoying. Treat D as an optional strict mode, not the default. Do not rely on merge-tree/conflict checks as a proxy; they catch only textual conflicts.

Scope / checklist

Decide scope: all protected branches, or only when checks_required is on.
Candidate ref namespace and lifecycle; GC must keep a candidate while a run needs it.
Enqueue candidate runs with the base branch filter; avoid duplicate runs when the candidate is unchanged.
Gate: require the candidate result for the landing tree, and keep the head-only path for FF.
Harden FindApprovedMatch with tree equality (independent of the rest).
Contributor allow-CI semantics for derived candidates: allow the head tip, then the candidate derived from it; record both.
UI states: untested, testing, passed, failed, stale base, conflict; Merge disabled until green.
Docs: change-requests “How approvals work”, workflows/runner “what runs”, change show; remove or qualify “merge, then push”.
Tests: gate candidate matching, tree verification, base-move invalidation, contributor policy, queue races.

Acceptance criteria

A non-FF landing is refused unless the required jobs are green for a commit whose parents are the current base tip and the request head, and whose tree is the landing tree.
An FF landing still needs only head checks and does not queue a duplicate run.
Moving the base invalidates the candidate result but not the head approval; no re-approval is required.
A manually pushed merge commit is accepted only if its tree equals MergeTree(parents) and its candidate checks pass; otherwise the push is refused with a message naming the mismatch.
Workflow definitions used for required checks come from the landing tree, so base-side workflow changes apply.
Candidate refs are server-only, excluded from branch listings, and do not survive indefinitely.
Contributor candidates run only after the exact head tip has been allowed; the allow record names the head and the derived candidate.
The request page and vole change show say whether the current landing tree has been tested, and the Merge control agrees with the gate.

Related: #11 (CI runner trust boundary), #20 (merge action), #45 (numbering).

Comments (1)

keenan 11d ago

Plan: automatic combined testing + merge when ready (A2-lite, plus C)

This supersedes the issue’s “C now, A1 as the minimum” recommendation. Under the constraint that neither the author nor a reviewer should have to take an extra step, A1 is not sufficient: it asks somebody to press Test merge, and to press it again after the base moves (“stale, test again”). The same constraint rules out D (rebase + re-approval churn) and E (still rebases). The useful shape is A, delivered as A2 but without the parts of a landing queue that only matter at high traffic.

The invariant is unchanged: the commit that lands is the commit that was tested, and a base move retests automatically without touching approvals.

What people have to do

Author: push the branch as usual. A base move never asks for a rebase. Only a real conflict does, because the server must not invent a resolution.
Reviewer: approve once. The approval names the head, not the base tip, so it survives base moves.
Merger: one Merge click. If the combined result is green it lands; if it is still running, that same click arms it to land when green. No second click.
Operator: nothing new. Candidate builds use the existing queue and runner pool.

When the combined result is built

This is the part revised after discussion. Approval is not what makes the combined result meaningful; it is only what makes landing possible. Testing earlier is review information, not speculation.

On head push (debounced): always build and test the candidate for any open request whose head is runnable. This is bounded by author activity, gives early integration feedback, and usually makes the result green before review finishes. A short quiet period (e.g. 2 minutes without another push) keeps a WIP burst from queueing a run per commit.
On base move: retest requests that are approved, armed, or viewed recently. This bounds the cost to requests somebody is actually working on, instead of open requests × merges.
On approval, Allow CI, or Merge click: kick the reconciler immediately. Usually a no-op, because the result is already there.
Dedupe by the candidate tuple. The ref name carries the base tip and the head, so publishing the same tuple twice costs nothing and two readers reuse the first writer’s commit.

A candidate derived from a contributor head still waits for the existing Allow CI decision for that exact head. That is not a new step: the head run needs it anyway, and the candidate derives its permission from the same record.

“Always” cannot guarantee the result is ready the instant an approval arrives: a base move between the green run and the approval invalidates it. Guaranteeing that would mean retesting every open request on every base move, which is the unbounded cost. Merge-when-ready makes that wait invisible, so no second action is required either way.

Making the candidate exist and be testable

One identity. C = commit(MergeTree(baseTip, head)) with parents [baseTip, head], published at refs/vole/candidates/<request-id>/<baseTip>-<head>. The ref name is the identity, so nobody has to recompute a commit hash. Creation is create-only: the first writer wins, later readers reuse its commit.
Why a ref is required. The runner fetches origin <sha> and an unreferenced commit is not fetchable. Publishing the ref makes its tip a fetchable want; verified against a real repository with no uploadpack.* config change. The ref is outside refs/heads/* and refs/tags/*, so it cannot be pushed and does not appear in branch listings.
Queue it. Enqueue the workflows at C (so base-side workflow changes apply) with Branch = req.Base, so on.push.branches filters and the gate see the landing branch.
Lookup vs publish. The pre-receive hook gets a read-only lookup: it finds the ref by tuple, verifies parents and tree, and checks its runs. It never writes during pre-receive. The server’s reconciler and Prepare own publishing.

The gate

If the new tip is a merge of the current base tip and the approved head, the tested SHA is the candidate, not m.Request.Head. CheckGaps(C, base) reads workflows at C, so a workflow change on the base applies to the landing.
A fast-forward keeps today’s path: C == head, no duplicate run.
Land re-runs the full gate at the moment it moves the ref, so an armed merge still rechecks approvals, permissions, and base freshness.

Close the pushed-merge hole (C)

In FindApprovedMatch, a pushed two-parent commit must also have the tree of MergeTree(oldTip, head). A merge carrying extra or different content is refused, naming the mismatch. If the tree matches, the gate checks the candidate results for that tree, so a hand-made merge rides the tested candidate instead of needing its own untested SHA. A human-resolved merge is a new head and is re-approved and re-tested; the server never resolves conflicts.

Trust and side effects

Three traps to close while doing this:

Cache namespace. The runner keys its cache namespace off the run’s ref; a candidate ref would otherwise use the trusted direct cache. Pin a trust value at enqueue (direct, or contrib-<name> for a head from a contributor branch) and have cacheFor use it.
Contributor provenance. A candidate must never be the first execution of a contributor commit. Require the exact head tip’s allow record, derive the candidate’s permission from it, and record both the head and the derived candidate.
No deploys from candidates. Clear PagesPath at enqueue for candidate runs; the runner’s branch-tip check would otherwise fail every candidate build of a pages job. Candidate runs are branch runs, not tag runs, so they publish no release.

ForSHA must include candidate refs alongside refs/heads/* (and must keep excluding tag runs, so a release build cannot satisfy a branch check).

Lifecycle

Delete candidate refs for a request when the tuple changes (once their runs are terminal) and on merge/close.
Older candidate runs are cancelled or left to finish; they no longer match a tuple and cannot satisfy the gate.
Deleting the ref is what makes the commit eligible for a future git GC. There is no server-side git GC today, so objects linger harmlessly until an operator runs one.

Merge when ready

Store a small intent (repo, request, actor) in the queue database.

Merge click: green lands now; testing arms; failed or conflicted refuses and names the reason.
The worker attempts Land on each new candidate result and re-runs the whole gate.
The intent survives base moves (it is dropped when the head changes, the request closes, it conflicts, approvals are lost, or the actor loses merge permission).
vole change merge behaves the same as the button, so the CLI and the page agree.

UI, CLI, docs

One new Combined result block: testing with the latest main, passed, failed (linked to the run), latest main moved and retesting, conflict on the branch, and “merging when the checks pass — cancel”.
The Merge control asks the same Prepare the write uses, and polls while a candidate is running or an intent is armed.
vole change show prints the same status.
Interface copy says “combined result” and “latest main”, not “candidate”, “base branch”, or “head”.
Update docs/change-requests.md (remove “merge, then push” as the recommendation), docs/workflows.md and docs/runner.md (“what runs”), and docs/cli.md (change merge when-ready).

Costs

Base moves cost one full run per approved/armed/active request; head pushes cost one per request that has actually pushed. If that becomes the bottleneck, B (a merge train) is the next step, not a reason to test less.
New state: candidate refs plus the auto-merge intent table and its cleanup.
Conflicts remain a human decision.

Order of work

C only — tree equality in FindApprovedMatch. Small, independent, closes a security-relevant hole.
Candidate core — publish/lookup the ref, ForSHA includes candidate runs, the gate checks C, fast-forward unchanged.
Triggering and testing — the reconciler and enqueue path with the revised trigger set, the trust/cache/no-deploy rules, and contributor derivation.
Merge when ready — intent table, worker, CLI/web parity.
UI, docs, tests — the states above; tests for candidate matching, tree verification, base-move invalidation, contributor policy, trigger debounce, and ref cleanup.

The first item is worth landing on its own even if the rest slips.