relctl: report who actually cut a release - #699
Open
Philip Lombardi (plombardi89) wants to merge 8 commits into
Open
relctl: report who actually cut a release#699Philip Lombardi (plombardi89) wants to merge 8 commits into
Philip Lombardi (plombardi89) wants to merge 8 commits into
Conversation
GitHub's actor on a release.yaml run names nobody who did anything.
release-prepare pushes the tag over SSH with RELEASE_TAG_DEPLOY_KEY rather
than GITHUB_TOKEN, because GitHub suppresses workflow triggers for tags
pushed with the default token, and GitHub attributes a deploy-key push to
whoever registered the key. So every release build since the key was
introduced reports the same person regardless of who cut the release.
triggering_actor is no help: on a push it equals actor, and on a re-run it
names whoever pressed re-run.
Correlate the build back to the release-prepare run whose execution window
contains the tag push, and report that run's dispatcher instead. watch gains
a "Cut by:" row and status a BY column.
Nothing in the API links a tag push to the run that made it, so this is
correlation rather than lookup. Two things it rests on:
- Windows are compared on created_at, never run_started_at. A re-run moves
run_started_at and leaves created_at alone, so created_at stays the
moment of the push however many times a build is retried. Run
33655638847 is the example: created 16:33:23Z by the push, attempt 6
started 18:29:20Z.
- Containment is unambiguous only because release-prepare has a
concurrency group with cancel-in-progress false, so prepares are
serialized and at most one window is open at a time.
Where it cannot tell, it says so. A hand-pushed tag has a truthful actor and
is reported as "Pushed by", which is worth distinguishing because such a tag
skipped every guard release-prepare applies. A build older than the
correlation window, or a soak inheriting its actor from the build, reports
unknown. The BY column never falls back to the raw actor: on a tag push that
value is the deploy key's owner, so printing it under a heading that reads as
"who did this" would state the one thing reliably known to be false. The raw
value stays available in -o json alongside the derived answer.
This is a workaround for the credential, not a fix for it. #698 tracks
replacing the user-owned deploy key with a GitHub App.
Attribute reads the absence of a matching release-prepare run as evidence that a tag was pushed by hand, and then reports the build run's actor as the person who pushed it. On a tag push that actor is the deploy key's owner, so the correctness of that reading rests entirely on the candidate list being able to contain the prepare in the first place. watch fetched the list before its poll loop, before any build existed. But `watch <tag>` is routinely started before the tag does - RELEASING.md documents `relctl cut` followed by `relctl watch <tag>` - and a list taken then cannot hold the prepare run that is about to push. Older prepares are still in it, so covers() passes, and the tag is reported as Pushed by: <deploy key owner> (tag pushed by hand, not by release-prepare) which is the exact claim this feature exists to prevent, asserted with more confidence than the raw actor ever carried: it additionally says the tag skipped every guard release-prepare applies. Defer the fetch to the first sighting of a build. The prepare pushed the tag, so it necessarily precedes the run the push created, and the race is gone rather than narrowed. Still fetched once, not per poll. status had the same shape without the same reachability: its two fetches are a second apart and a prepare takes minutes to reach its push, so no prepare missing from the earlier list could have pushed anything yet. Reorder it anyway, for the ordering rule to hold everywhere and for a plainer reason - "Nothing in flight." is the ordinary answer for a dashboard people re-run, and there is nobody to attribute then. Collecting the runs first turns that case into no request at all. Nothing can check the ordering from inside Attribute: a list fetched an hour too early looks exactly like one fetched a second too late. So the obligation is documented on Prepares and pinned by tests that assert the request order, which fail in CI rather than degrading in silence. Also stop letting a failed candidate fetch kill either command. Nil is a safe list to attribute against - nothing contains a build and nothing covers one - so every run degrades to unknown. watch says which kind of unknown it is rather than blaming the tag for a failed request, and status says it once under the column instead of leaving four "?" to read as four findings.
A prepare run's window is open from when it was created until shortly after it finished, whatever it finished as. So a prepare that failed still claims any tag push landing inside it - and reports the person who dispatched a run that never got as far as pushing anything. Everything after `git push origin "$TAG"` in release-prepare.yaml is an echo and a step summary write, and the only later step is skipped on a real cut. A prepare that failed therefore failed before its push with near certainty, and cannot be what created the tag. The tag was pushed by hand, and the run's actor is the person who pushed it. Exclude only what GitHub reported as not succeeding, via a new Run.Failed rather than !Succeeded(). A completed run with no conclusion renders as "completed" and is an absence of evidence, not a failure; a run still in progress may be a step away from its push. Excluding either drops a prepare that did push, and the fall-through from that is naming the deploy key's owner - the outcome the rest of this file is arranged to avoid. That is not a hypothetical distinction: the pre-existing "newest containing prepare wins" fixture has completed runs with no conclusion, and !Succeeded() broke it. The exclusion applies to containment only, never to covers(). A failed prepare still proves the candidate list reaches back past the build, which is what makes a non-match mean "pushed by hand" rather than "we did not look far enough". Dropping it there would shrink the window and push honest hand-pushed tags into unknown. Dry runs remain unfilterable - they succeed and push nothing, and the API does not expose a run's dispatch inputs - but reaching that case needs a tag pushed by hand inside a dry run's window, and its dispatcher is the likely answer anyway.
A watch runs for up to ninety minutes, polling every twenty seconds across three endpoints. That is on the order of a thousand requests, and until now any single one of them coming back wrong ended the watch - which made the command least reliable exactly when a release was taking a long time, the case it exists for. Retry the ones that could pass later and report the ones that could not. The line is drawn on go-github's typed errors rather than on status codes, because GitHub answers a rate limit with 403, the same code it uses for "you may not do that". Only the type separates a wait from a dead end. A 5xx is GitHub saying the fault is theirs; an error that never reached GitHub taught us nothing about the request; both are worth another go. A 401, 403 or 404 will say the same thing in ninety minutes, and retrying it turns a clear error into a timeout. Cancellation is checked first. It arrives through the same return as an API failure and would otherwise look like a network blip, turning a Ctrl-C into a retry loop. --once does not retry. It is documented to report current state and exit, so a caller who asked one question gets one answer, never a wait. Retries are announced on stderr, never stdout, which carries the JSON that -o json callers parse. And giving up now names the cause: a watch that spent its timeout being told 502 was previously indistinguishable from one that spent it waiting for a release that never came, since both said only "gave up waiting for v0.5.0 after 1h30m0s". Transient is exported and tested against the real call paths rather than hand-built errors, because every wrapper in this package annotates with %w and the classification depends on it. A wrapper that switched to %v would still compile, still read fine, and would quietly restore the old behavior.
Two places where the prose and the code disagreed. watchResult.By is documented as carried on the result "rather than on Build", because who cut a release is a fact about the TAG: the build and every soak carry the same distorted actor, so attaching the derived answer to each would repeat one finding several times and read as several observations. The code then set it on Build as well, so -o json emitted it twice. Drop it there and keep the raw actor, which is what the Actions UI shows and what anyone reconciling the two needs. A test pins the shape so it stays a decision. The README claimed the BY column "never falls back to the raw actor". That property is real but it lives in covers(), not in the rendering - and for a tag genuinely pushed by hand the column does show the actor, correctly, since there the actor is the person who pushed. Stated as a property of the column it misdescribes where the guarantee is, which is how a guarantee gets refactored away. Say what actually holds: a name appears only where the derivation reached one, the fallback is "?" rather than the actor, and the candidate list is fetched after the runs it explains. Also document the failed-prepare exclusion and the degrade-on-failure behavior, both of which arrived in this branch without a word in the README.
`cancelled` is what the workflow runs API returns as a run conclusion, alongside `failure`, `timed_out` and `success`. misspell with locale US rewrites it to `canceled`, which is not a value GitHub ever sends. The PR that introduced attribution predicted this would eventually be needed and left it for whoever first had to compare against that conclusion. Adding a failed-prepare filter is that moment: its fixtures carry the literal, and without an exclusion `make fmt` silently turned them into runs the API cannot produce. The tests went on passing, because the code under test matches on "not success" rather than on the word - so the only thing lost was the fixture's claim to describe a real response, which is exactly the property a fixture exists for. The next reader to write `Conclusion == "cancelled"` would have had it rewritten out from under them with no failure at all. Scoped to the literal. Prose about it is American like everything else, so the test case names say "stopped" and the comments spell it the American way. Keeping the exclusion this narrow means broadening it later has to be a decision somebody makes rather than a line that quietly already covers it. Also mark the retry note in watch as deliberately unchecked. errcheck runs with check-blank, so `_, _ =` is not an escape either, and that is the right policy: this is the one write in the file that should not fail the command, because it is an advisory line on the diagnostic stream about a failure already being recovered from.
Cameron Childress (cchildress)
approved these changes
Sep 3, 2026
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.
What
relctl watch <tag>gains aCut by:row andrelctl statusaBYcolumn,naming who actually cut a release.
Why
release-preparepushes the tag with a deploy key rather thanGITHUB_TOKEN,because GitHub suppresses workflow triggers for tags pushed with the default
token. GitHub credits a deploy-key push to whoever registered the key, so
every release build names that same person.
triggering_actoris no better: on apush it equals
actor, and on a re-run it names whoever pressed re-run.Checked across all 80
release.yamlruns: every build with a matching preparereports the key owner, including releases cut by
bcho,vpatelsj,jwilder,jveskiandcchildress.How
Nothing in the API links a tag push to the run that made it, so this is
correlation rather than lookup: find the
release-preparerun whose executionwindow contains the push and report who dispatched it.
gh.Attributeis pure andseparate from the fetching for that reason.
Cut by: <login>with a prepare linkPushed by: <login>Cut by: unknown, or?inBYWhat it rests on, all commented in the code:
created_at, neverrun_started_at. A re-run moves the latterand leaves the former at the moment of the push.
as "pushed by hand", so a list taken too early would name the key owner as the
pusher.
release-prepareis serialized by aconcurrencygroup.thing that job does.
BYnever falls back to the raw actor as a default. The raw value stays in-o jsonasactor, beside the derived answer underby.watchalso stops ending a 90-minute poll on one bad response: 5xx, rate limitsand connection failures are retried, definite answers like a 404 still fail at
once, and
--oncenever retries.Testing
Table and named tests in
gh/attribution_test.go, including that no path canreport the key owner and that correlating the same fixture on
run_started_atfinds nothing. Command-level tests assert the request order, which no
inspection of the output can show. Mutation-checked. Verified live:
v0.6.0→cchildress,v0.4.0→plombardi89,v0.2.3-alpha.2→ a hand push bybcho,v0.1.5→ unknown.Notes
A workaround for the credential, not a fix. #698 tracks replacing the deploy key
with a GitHub App — though an App token names the app, so it makes
actorhonestrather than correct.
The
.golangci.yamlchange stopsmake fmtrewritingcancelled, which isGitHub's spelling for a run conclusion, to a value the API never sends.