feat(runway): credit the change author on squash and merge commits - #482
Merged
Conversation
This was referenced Jul 31, 2026
behinddwalls
force-pushed
the
preetam/runway-uri-unit-docs
branch
from
July 31, 2026 23:13
a4fbfd9 to
7f67749
Compare
behinddwalls
force-pushed
the
preetam/runway-pr-author
branch
from
July 31, 2026 23:13
6a019a1 to
bec528c
Compare
behinddwalls
marked this pull request as ready for review
July 31, 2026 23:13
behinddwalls
force-pushed
the
preetam/runway-pr-author
branch
from
August 1, 2026 17:30
bec528c to
9b3e5f9
Compare
behinddwalls
force-pushed
the
preetam/runway-uri-unit-docs
branch
from
August 1, 2026 17:30
7f67749 to
592de81
Compare
kevinlnew
approved these changes
Aug 5, 2026
behinddwalls
added a commit
that referenced
this pull request
Aug 6, 2026
## Summary ### Why? The git merger exists but nothing constructs it — the server still builds the noop factory, so a deployed Runway acknowledges every merge as an instant success. This is the change that makes Runway actually merge. It is deliberately last in the stack and deliberately opt-in: the switch is the presence of `MERGE_CHECKOUT_PATH`. Unset, the server keeps wiring noop, which is what local development, the compose stack, and the e2e suite depend on — none of them have a git checkout to hand the merger. ### What? `newMergerFactory` now reads the environment and returns either backend. With `MERGE_CHECKOUT_PATH` set it builds a git merger from the `MERGE_*` / `GIT_*` variables — remote, target branch, default strategy, committer identity, and the pinned git runtime — and logs the resolved configuration at startup. Without it, noop, with a log line saying so. A malformed configuration fails startup rather than degrading silently: `parseStrategy` rejects an unrecognized `MERGE_DEFAULT_STRATEGY`, and `DEFAULT` itself is rejected because it cannot be the value a `DEFAULT` step resolves to. Three settings govern the behaviors the merger cannot infer: - `MERGE_CHECK_STALENESS` (default on) verifies each change's provider ref still points at the commit its URI names before applying. - `MERGE_ALLOW_UNRELATED_HISTORIES` (default off) lets a MERGE step import a history that shares no ancestry with the target. It stays off because the refusal it lifts is a safeguard everywhere except a queue whose purpose is such imports. - `MERGE_FETCH_REFSPECS` supplies extra refspecs for a remote that will not serve an unadvertised commit by SHA. Normally empty. `gitMergerFactory` hands the same merger instance to every queue. The merger owns one checkout and serializes its own operations, so a second instance over the same directory would race; a deployment that lands multiple targets wires a per-queue map instead. This is also why the factory is built once at startup rather than per request. ## Test Plan ✅ `bazel build //service/runway/...` — wiring compiles ✅ `bazel test //runway/...` — all targets pass Not covered by automated tests: the git path only engages when `MERGE_CHECKOUT_PATH` points at a real checkout, so the env-to-`Params` mapping is exercised by the merger's own suite rather than through `main`. Watch the `git merger configured` startup log — with checkout, target, and default strategy — on the first deployment that sets the variable; its absence means the server silently fell back to noop. ## Issues ## Stack 1. @ #463 1. #476 1. #477 1. #482
behinddwalls
force-pushed
the
preetam/runway-uri-unit-docs
branch
from
August 6, 2026 00:32
592de81 to
06211d8
Compare
behinddwalls
force-pushed
the
preetam/runway-pr-author
branch
from
August 6, 2026 00:32
9b3e5f9 to
3f013b5
Compare
behinddwalls
force-pushed
the
preetam/runway-uri-unit-docs
branch
from
August 6, 2026 00:40
06211d8 to
64a8264
Compare
behinddwalls
force-pushed
the
preetam/runway-pr-author
branch
from
August 6, 2026 00:40
3f013b5 to
6cf0584
Compare
behinddwalls
added a commit
that referenced
this pull request
Aug 6, 2026
## Summary ### Why? The merger decides which provider a change came from by its URI scheme, and rejects a scheme it has no parser for. That part works, and it happens before any git command runs. What it does not do is check that the changes in one request agree with each other. `resolveChange` determines the provider per URI and then discards it, so a request whose steps are addressed through different providers is resolved by different parsers and applied as though nothing were unusual. SubmitQueue already refuses that within a single change, but a Runway request carries one step per SubmitQueue request, so nothing covers the request as a whole. ### What? Keeps `Provider` on `changeRef` — the scheme the change was addressed through — rather than parsing it and throwing it away. `resolveAndValidate` now compares every change against the first and rejects a request that mixes providers, naming both and the steps they came from. It already walked every URI to validate it, and it runs before the mutex and before any git command, so an incoherent request costs nothing and leaves the checkout untouched. This cannot refuse a legitimate request: there is no way to address one merge through two providers, and the apply paths would otherwise have to reason about changes resolved by different parsers. Also names the change, not just the commit, in the unavailable-commit error, so the reader is not sent looking for a deleted commit when the likelier cause is a change this remote was never going to serve. Whether a change belongs to the repository this merger serves is deliberately not checked. The merger is already constrained to its checkout and remote by configuration, and a change it cannot fetch is refused on those grounds. ## Test Plan ✅ `bazel test //runway/...` — all targets pass (git suite 70s) ✅ `make lint`, `make check-tidy`, `make check-gazelle`, `make test` New cases: two steps using different providers, one change spanning two providers, and an unsupported provider — each asserted terminal and not a conflict. A multi-step multi-URI request through one provider is asserted to still succeed, guarding against over-rejecting. The rejection cases run against a Merger whose git executable does not exist, so any git invocation would fail as an exec error. Getting `ErrInvalidRequest` back proves the request was refused before the merger reached for git. ## Issues ## Stack 1. #463 1. @ #476 1. #477 1. #482
behinddwalls
force-pushed
the
preetam/runway-uri-unit-docs
branch
from
August 6, 2026 01:01
64a8264 to
744dceb
Compare
behinddwalls
force-pushed
the
preetam/runway-pr-author
branch
from
August 6, 2026 01:01
6cf0584 to
976aac2
Compare
behinddwalls
added a commit
that referenced
this pull request
Aug 6, 2026
## Summary ### Why? The contract never said what a URI is relative to a change, and the two readings lead to different behavior. Under one, a change is the unit and its URIs are pieces of it; under the other, each URI is a change and a list is a stack of them. Nothing wrote the second one down, so the first kept getting assumed — most recently in SQUASH_REBASE, which collapsed every URI of a step into one commit and erased the boundary between stacked pull requests. The strategy fields had the same hole. Nothing said whether a strategy is picked once and repeated per URI, or picked once for the list as a whole. Both `LandRequest.strategy` and `MergeStep.strategy` are singular, which reads either way. ### What? States the rule once where a change is defined, in `uber.base.change.Change` and its `platform/base/change` entity: one URI is one unit of change, and a list is an ordered set of distinct changes applied each on top of the last, not one change described several ways. Carries it to the places a strategy is chosen. The `Strategy` enum now says a strategy applies to every URI the change carries, the same way to each, and its values are defined per URI — so `SQUASH_REBASE` says the squash unit is the individual change, and a stack of three URIs becomes three commits rather than one. `PROMOTE` notes that it is the one value constraining the list rather than repeating over it: advancing a ref to an exact revision admits a single URI, because a second could not also be the revision the target ends at. The two call sites say the same in their own terms — `MergeStep.strategy` that a step is never a mix of strategies, `LandRequest.strategy` that a land request cannot pick a different strategy per URI — as do the `LandStrategy` entity fields. The git merger's README gains the corresponding implementation statement: the URI is the unit of application, and a step's outputs are the concatenation of what each URI produced. Documentation only. The behavior described is what the code already does; this is the contract catching up with it, so that the next reader does not have to infer the rule from an implementation. ## Test Plan ✅ `make proto` — regenerated stubs carry the new comments ✅ `bazel test //runway/... //service/runway/... //submitqueue/...` — 47/47 ✅ `make lint`, `make check-tidy`, `make check-gazelle`, `make test` No behavior change, so no new tests. The rule stated here is already pinned by existing cases — `TestMerge_SquashRebase_OneCommitPerChange` for the per-URI squash unit, `TestMerge_RejectsInconsistentProvider` for one provider per request, and the PROMOTE composition cases for its single-URI constraint. ## Issues ## Stack 1. #463 1. #476 1. @ #477 1. #482
## Summary ### Why? Git records an author and a committer separately, and the merger was collapsing the two: `command()` pins `user.name`/`user.email` to the configured merger identity on every invocation, so any commit the merger creates is both authored and committed by `SubmitQueue Runway <runway@submitqueue.invalid>`. A landed change shows the service, not the person who wrote it — in `git log`, in blame, and in every tool built on them. `REBASE` was never affected: cherry-pick carries each commit's author across on its own. The strategies that mint a fresh commit are the ones that lose it — `SQUASH_REBASE` builds its commit with `reset --soft` + `git commit`, and `MERGE` creates a `--no-ff` merge commit. `PROMOTE` creates no commit at all, so there is nothing to attribute. ### What? The committer stays the merger — it is what applied the change, and that is what a committer means. The author now comes from the change: the author recorded on the commit its URI pins. For a change spanning several commits by different people that is the head commit's author, which is the one identity the request actually names. The author is read out of the local object store (`git show --no-patch --format=%an%x00%ae`), so this needs nothing on the wire and costs no network — every referenced commit is already fetched and verified before a step is applied. No change to the `Change` proto, and no resolver, keeping the property that the merger reads change URIs straight from the payload. A commit recording no usable author (either half missing) falls back to the committer identity rather than failing the merge. The identity travels through `GIT_AUTHOR_NAME`/`GIT_AUTHOR_EMAIL` rather than `git commit --author`. Two reasons: `git merge` has no `--author` flag, so the merge commit could not use one; and the environment carries the name and address as separate values, where `--author` takes a single `Name <address>` string that git parses back apart — a display name containing an angle bracket splits in the wrong place, and one that parses to nothing makes git search history for a matching author and fail the commit outright. New `author.go` holds `authorIdent` and `commitAuthor`. `run`/`runCombined`/`command` gained `…As` variants taking an author; the existing signatures delegate to them with a zero author, so every other call site is unchanged. ## Test Plan ✅ `bazel test //runway/...` — 6/6 targets pass. New tests in `git_merger_test.go`: squash credits the change author, the `--no-ff` merge commit credits the change author, a multi-author change credits its head commit's author, and `REBASE` keeps each commit's own author. Every one also asserts the committer is still the merger, which is the invariant that separates attribution from impersonation. `TestAuthorIdent` covers the fallback when either half is missing, and a name containing angle brackets surviving verbatim. Verified the tests actually catch the bug: reverting just the two attribution call sites fails exactly the three attribution tests and leaves the `REBASE` one passing, which is the expected profile since `REBASE` was already correct. Ref: the equivalent change on the gitfarm merger, `uber-code/go-code` 454437cc — same intent, different mechanism, because that merger resolves the PR author over RPC where this one has the commits locally.
behinddwalls
force-pushed
the
preetam/runway-pr-author
branch
from
August 6, 2026 01:02
976aac2 to
e43bc78
Compare
behinddwalls
temporarily deployed
to
stack-rebase
August 6, 2026 01:03 — with 
GitHub Actions
Inactive
GitHub Actions
Inactive
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Why?
Git records an author and a committer separately, and the merger was collapsing the two:
command()pinsuser.name/user.emailto the configured merger identity on every invocation, so any commit the merger creates is both authored and committed bySubmitQueue Runway <runway@submitqueue.invalid>. A landed change shows the service, not the person who wrote it — ingit log, in blame, and in every tool built on them.REBASEwas never affected: cherry-pick carries each commit's author across on its own. The strategies that mint a fresh commit are the ones that lose it —SQUASH_REBASEbuilds its commit withreset --soft+git commit, andMERGEcreates a--no-ffmerge commit.PROMOTEcreates no commit at all, so there is nothing to attribute.What?
The committer stays the merger — it is what applied the change, and that is what a committer means. The author now comes from the change: the author recorded on the commit its URI pins. For a change spanning several commits by different people that is the head commit's author, which is the one identity the request actually names.
The author is read out of the local object store (
git show --no-patch --format=%an%x00%ae), so this needs nothing on the wire and costs no network — every referenced commit is already fetched and verified before a step is applied. No change to theChangeproto, and no resolver, keeping the property that the merger reads change URIs straight from the payload. A commit recording no usable author (either half missing) falls back to the committer identity rather than failing the merge.The identity travels through
GIT_AUTHOR_NAME/GIT_AUTHOR_EMAILrather thangit commit --author. Two reasons:git mergehas no--authorflag, so the merge commit could not use one; and the environment carries the name and address as separate values, where--authortakes a singleName <address>string that git parses back apart — a display name containing an angle bracket splits in the wrong place, and one that parses to nothing makes git search history for a matching author and fail the commit outright.New
author.goholdsauthorIdentandcommitAuthor.run/runCombined/commandgained…Asvariants taking an author; the existing signatures delegate to them with a zero author, so every other call site is unchanged.Test Plan
✅
bazel test //runway/...— 6/6 targets pass.New tests in
git_merger_test.go: squash credits the change author, the--no-ffmerge commit credits the change author, a multi-author change credits its head commit's author, andREBASEkeeps each commit's own author. Every one also asserts the committer is still the merger, which is the invariant that separates attribution from impersonation.TestAuthorIdentcovers the fallback when either half is missing, and a name containing angle brackets surviving verbatim.Verified the tests actually catch the bug: reverting just the two attribution call sites fails exactly the three attribution tests and leaves the
REBASEone passing, which is the expected profile sinceREBASEwas already correct.Ref: the equivalent change on the gitfarm merger,
uber-code/go-code454437cc — same intent, different mechanism, because that merger resolves the PR author over RPC where this one has the commits locally.Issues
Stack