relctl: one implementation of the versioning rule, shared by humans and CI - #655
Conversation
RELEASING.md:150 says of deciding whether main is releasable: "There is no single dashboard." Nor is there one for what is in flight, which trains are live, or where a release has got to. This is the start of one. Auth prefers an existing gh login over a provisioned token. Every maintainer who can cut a release already has gh working, and requiring a PAT to run a read-only status would be the friction that stops a tool being used. GITHUB_TOKEN and GH_TOKEN are checked first, because a workflow always has one and may not have gh installed at all; a test pins that order, since reversing it would work locally and fail only in CI. The client is built per command rather than at the root. Version resolution is pure git and has to keep working with no credential, in a clone or in a workflow that was never granted one.
The versioning rule moves first because it is the smallest piece and the one everything else depends on: main cuts minors and majors, release-X.Y cuts patches, and nothing else may release at all. Two test suites, and the distinction matters. The ported table cases are the coverage, and have to keep protecting this once the shell is gone. On top of that, a differential test runs both implementations over 62 inputs and asserts they agree on the answer and on whether they refused at all. That corpus is deliberately wider than either implementation's own cases, since the cases encode what we thought to test and the point is to catch what we did not: unicode digits, embedded newlines, prefixed and suffixed branch names, a nine-digit ceiling. The differential test is temporary and goes when the shell does. Proved it bites first: relaxing the regex to allow leading zeros fails exactly on release-01.2, which is the case the shell refuses because v01.2.3 is not a version and that series could never match a tag.
408 lines of bash that decide which tag a release mints and at which commit. Ported function by function rather than rewritten, because the comments record why each rule exists and several of them are scar tissue: rc.18 losing to rc.9 under a lexical maximum, rc.08 read as octal and silently skipped, promote guessing between trains and orphaning v0.1.24 at rc.18. Two suites, and the distinction is the whole plan. resolve_test.go carries all 76 fixtures from next-version-test.sh and is the coverage that has to keep protecting this once the shell is gone. The differential test is a temporary equivalence proof and goes with it. The cases were not transcribed. bash parsed its own suite's expect invocations and emitted the Go table, so the fixtures and expectations are the shell's, not my reading of them. All 76 passed against the Go implementation on the first run. The differential test compares 345 resolutions: the 76 ported fixtures, 62 branch-policy inputs, and 207 generated combinations of finals, candidates, off-branch tags and malformed metadata across every mode and bump. The generated half matters more than the ported half. The written cases encode what we expected to matter; a port goes wrong in the shapes nobody considered. Proved it bites before trusting it. A minor bump that forgets to reset patch fails three cases; dropping reachability scoping from tag discovery fails the release-branch cases, which is exactly the property that keeps a release branch inside its own series. git is behind an interface so the resolver is a pure function of a tag set, and Repo.TagExists is deliberately repository-wide while tag discovery is reachability-scoped. Those answer different questions: one is "is this name taken anywhere", the other "what has this branch released". Collapsing them is how a train gets started on a core that can never be promoted.
Philip Lombardi (plombardi89)
left a comment
Philip Lombardi (plombardi89)
left a comment
There was a problem hiding this comment.
Read the full diff against both shell originals, the workflow that still calls them, the NOTICE collector and the lint config.
The port is faithful. I walked every branch of next-version.sh against resolve.go/modes.go and found no divergence in the resolved tag or base. Report ordering matches, mode dispatch matches, the validation sequence (shape, series, tag-exists, ancestry, describe) matches, and both pieces of scar tissue survive intact: numeric max_rc, and repository-wide has_final kept distinct from reachability-scoped discovery.
The findings below are not in the port. They are in the evidence, and in what survives the oracle's deletion.
Three I would want settled before the shell goes:
- 34 of the 76 permanent cases assert only that an error happened.
branch_test.gomatches on the message; the resolve cases do not. That is 45% of the suite that would pass if every refusal collapsed into one. Live,Stale,LatestFinal,ReportandWarningshave no assertions in either suite. The differential comparesTagandBaseonly. The live/stale model is what the rest of the CLI is built on, and it is the exact thing the shell got wrong with the v0.1.24 orphan.- The differential runs the shell with
os.Environ().next-version.shreadsBUMP/SERIES/PRE/VERSION/ALLOW_CONCURRENT_TRAINSwith:-defaults, andrequest()builds the Go side fromtc.envonly. "345 comparisons, all agreeing" holds on a clean environment; the generated 207 set onlyBUMP=.
A fake Repo in resolve_test.go is the cheapest route: it closes 1 and 2, removes the ambient-config exposure in the first inline comment, and cuts the local test loop substantially. The differential genuinely needs real git; the permanent suite does not.
The gh package landing ahead of its callers reads as deliberate (dependency approval settled early, command commits stay small). Noting it only so a later reviewer does not re-raise it.
Nits not worth their own thread: relctl.go:32 defer stop() is unreachable before os.Exit(1); the strconv.Atoi error branch in prerelease is dead since preShape already bounds to nine digits, and unlike parseCore it does not say why it is kept; the NOTICE commit message says it picks up "go-github and its go-querystring transitive", but hack/cmd/notice/internal/gomod/gomod.go:35 is direct-deps-only and one entry was added, so the file is right and the message is not; SplitRepo defends the URL paste explicitly but accepts Azure/un bounded; Options.BaseURL is documented "for tests" with no test using it.
Worth considering putting the differential behind a build tag or testing.Short(). It is a temporary equivalence proof and it is currently paid for on every local make test, which also implies lint.
notice-check runs in CI, so adding a module without this turns the build red. One entry: hack/cmd/notice collects direct dependencies only, so the transitive go-querystring does not appear.
Review found the evidence weaker than the port. Two gaps, both in what survives the oracle's deletion rather than in the resolution itself. 34 of the 76 cases asserted only that an error happened. That is 45% of the suite passing if every refusal collapsed into one message, and the reasons ARE the behaviour here: refusing a core whose final exists off this branch is a different fact from refusing a version outside its series. Each refusal now names its reason, and a guard test pins the number of DISTINCT reasons, because per-case substrings alone would not notice them converging. Live, Stale, LatestFinal, Report and Warnings had no assertions at all. That is the model status, prerelease and promote are built on, and the one the shell historically got wrong: v0.1.24 reached rc.18 and was orphaned when a v0.2.0 train started beside it. trains_test.go asserts it directly, including that shape. The suite now runs against a fake Repo rather than real git. That is what makes the train model assertable without scraping output, drops any dependence on the developer's git config, and takes the loop from 630ms to 6ms. The fixtures keep their meaning: the fake is built from the same @new and @off specs. A fake the suite trusts is a fake that can drift, so the next commit makes git prove it.
Two things the previous commit left owing. The permanent suite now runs against a fake Repo, so nothing checked the fake against git. The differential is the only other code touching real git and it is deleted at the same time as the shell, which would have left the suite testing the fake rather than the resolver. TestFakeRepoMatchesGit resolves the same fixtures through both and requires the same tag, latest final, live and stale, and the same base IDENTITY. Hashes cannot be compared, so the base is compared by what it points at: HEAD, or the tag whose commit it is. Confirmed it bites by making the fake ignore reachability, which is the drift that would matter, and it fails. The differential ran the shell with os.Environ(). next-version.sh reads five variables with `:-` defaults while the Go side is built from the case alone, so the two implementations were being fed different inputs and "345 comparisons agreeing" was a statement about my shell. Exporting SERIES=0.3 made 33 cases disagree. Every variable the script reads is now set explicitly, defaulted to match Request, with only PATH and HOME inherited. It passes with SERIES, BUMP, PRE, VERSION, ALLOW_CONCURRENT_TRAINS and MAJOR all set hostilely. The proof also compared the tag and base only, which left the train model outside it. It now parses Latest final, Live trains and Stale trains from the shell's report and compares those too. That is not cosmetic: swapping live and stale does not change the computed tag, so the old differential could not have seen it. It now fails 77 cases.
From review:
- defer stop() sat above an os.Exit(1), which skips defers, so the
signal handler was left uninstalled on every failing invocation. The
work moves into run() returning an exit code.
- The strconv.Atoi branch in prerelease is unreachable, since preShape
already bounds the suffix to nine digits with no leading zero. Kept,
but it now says why, matching how parseCore treats the same
situation. Silence was the actual complaint.
- SplitRepo defended against a pasted URL but accepted "Azure/un
bounded". It now validates the shape GitHub allows, so a mistyped
argument fails there rather than as a 404 that reads like a
permissions problem.
- Options.BaseURL was documented "for tests" and used by none. An
httptest case now exercises it, which is what would notice if
go-github changed how a base URL is applied; without it the first
symptom would have been a suite quietly talking to the real API.
The exit-code work turned up something the review did not: `relctl
bogus-subcommand` printed help and exited ZERO. cobra skips argument
validation entirely for a command it cannot run, so Args: cobra.NoArgs
did nothing on a root with no RunE. A mistyped release command looking
like success is worse than any of the above, and it is the sort of fault
nothing notices, since the tool appears to work having done nothing.
Root now has a RunE, and three tests pin unknown-subcommand, bare
invocation and the --repo default.
01d8f5b to
591ff17
Compare
|
All three settled, plus the nits. Verified each finding before acting on it. 1. 34 refusals asserting only that an error happenedConfirmed: 34 of 76, 44%. Every refusal now names its reason. On its own that is not enough — per-case substrings would not notice the reasons converging — so 2. No assertions on the train modelConfirmed: both suites asserted
I also extended the differential to compare the state model, not just the Go suite. That turned out to matter more than expected — swapping live and stale does not change the computed tag, so the old proof could not have seen it. It now fails 77 cases. 3. The differential inheriting the environmentConfirmed, and it reproduces exactly as you describe: Every variable the script reads is now set explicitly, defaulted to match The proof: Since fixing input plumbing is exactly the change that can neuter a test, I re-confirmed it still bites afterwards: a minor bump that forgets to reset patch fails 31 cases. The fake Repo, and the gap it openedTaking your suggestion closed 1 and 2 and dropped the permanent suite from 630ms to 6ms. It also created a hazard worth naming: with the permanent suite on a fake, and the differential — the only other code touching real git — deleted alongside the shell, nothing would check the fake against git. The suite would become a test of the fake.
NitsAll four taken. That work turned up something worse than any nit. Not takenGating the differential behind a build tag or The NOTICE commit message was wrong, as you spotted: You are right that the State501 assertions in |
classify-release.sh answers two questions that are not the same
question, and an earlier design answered both with one version
comparison and got the first one wrong.
FromMain provenance, not ordering. unbounded-stable soaks main and
only main. Ordering cannot answer this: the cluster can be
running a candidate newer than the newest final, so an
ordering test says deploy when the honest answer is that a
release-branch release has no business there at all.
Latest ordering, because Latest is what releases/latest/download
resolves to, which is the install command in README.md and
every guide.
The Latest half needs true semver precedence, which is why the shell
delegated it to hack/cmd/semver: sort -V ranks v1.0.0 BELOW v1.0.0-rc.1
where clause 11.3 requires the opposite. That tool has no other caller,
so it is absorbed here and deleted with the shell. It uses x/mod/semver,
already a dependency.
Deliberately NOT reusing greaterFinal, which refuses prereleases for
exactly that reason. Both ends now say so, because relaxing greaterFinal
later to "simplify" the two into one would silently invert the resolver.
Repo grows AllTags alongside ReachableTags. They answer different
questions and conflating them breaks in both directions: discovery must
be reachability-scoped so a stray tag on someone's branch cannot drive
the numbering, and the Latest query must NOT be, because a release
branch's own tags are invisible from main and are exactly what could
outrank it. Mutating the second one away fails the backfill cases in
both suites, which is the bug that flips the Latest marker backwards.
The tag pattern stays looser than the resolver's, accepting alpha and
beta: this classifies what was actually tagged, and a historical tag
must remain readable rather than merely unfashionable.
7 ported cases plus refusals, and an 18-comparison differential. The
oracle needed a prebuilt semver binary rather than the script's `go run`
default, which resolves against the working directory and would look for
a module inside the fixture.
status, next, preflight and watch. RELEASING.md:150 says of deciding whether main is releasable: "There is no single dashboard." preflight is that; status is the same for the release itself. The hard part is watch. Three workflows are involved and only the first names the tag: a tag push sets release.yaml's head_branch to the TAG, so those runs are identifiable. release-upgrade fires on workflow_run, reports the default branch, and the API exposes no link back to what triggered it. Matching on head_sha alone is ambiguous, and not theoretically: a promoted final and its last candidate share a commit. v0.3.0 and v0.3.0-rc.1 are both 3c9621d, as are v0.2.4 and v0.2.4-rc.1. Both tags therefore produce build runs with that sha, and so do their soaks. So the sha narrows server-side and a TIME WINDOW disambiguates: a soak belongs to this build if it started after it and before the next build of the same commit. Manual workflow_dispatch retries land inside the window, which is right, since they are attempts at the same soak. Tested against a stub that filters the way GitHub does, because a stub ignoring head_sha would exercise a path that cannot happen and hide one that can. Removing the time window fails those cases. Verified against the real repository too: watch v0.4.0 resolves build 32747498643 and soak 32749611291, which are the runs it actually used. next carries a github output format emitting byte-identical tag=/base= lines, so release-prepare can call it in place of the shell without its downstream steps changing. A test pins that to exactly two lines: anything extra lands in $GITHUB_OUTPUT and can corrupt it. The GitHub client is built per command rather than at the root, and a test pins that next works with no credential at all. Version resolution is pure git, and it has to keep working in a workflow that was never granted a token. Running status against the repository turns up 24 draft releases going back to v0.1.17, each a release that built and never shipped. Reported with that explanation rather than as a bare list.
Philip Lombardi (plombardi89)
left a comment
Philip Lombardi (plombardi89)
left a comment
There was a problem hiding this comment.
Second pass, over e14bb31e..1da4c2a6.
The previous round was addressed properly
| Prior finding | Status |
|---|---|
| Error identity | Fixed. All 34 refusals carry wantErr, plus TestEveryRefusalNamesADistinctReason as a meta-guard. That guard is a better idea than what I asked for: per-case substrings alone would not notice the reasons converging. |
Live/Stale/Report untested |
Fixed. trains_test.go asserts the model directly, and the differential now parses the shell's stderr report and compares it too. "Swapping live and stale does not change the computed tag" is the right observation, and 77 cases now fail if you do. |
| Env leak in the differential | Fixed, and verified rather than assumed. "Exporting SERIES=0.3 made 33 cases disagree" establishes the bug was real, not theoretical. |
Repo interface unused |
Fixed, and better than asked. The fake landed with TestFakeRepoMatchesGit, comparing base by identity rather than by hash. I did not ask for the conformance test; it is the thing that makes the fake safe to trust, and mutating reachability out of it to prove it bites is the right check. |
| Nits | defer stop(), the Atoi branch, SplitRepo, BaseURL all handled. The cobra NoArgs bug found along the way is a worse fault than anything I raised: a mistyped release command exiting zero. |
One prior finding is only partly closed, and two are now load-bearing in a place they were not before. Both are inline.
New findings
The two that matter are classify.go failing open toward Latest = true on a git error, and watch never ending on a failed soak. Both inline.
Behind those, a coverage question. cmd_status.go, cmd_preflight.go, cmd_watch.go and gh/releases.go are 767 lines with no test files, and relctl.Options has no seam to add them through. Given this PR spent three commits making the resolver's evidence hold up, the command layer arriving untested stands out.
Nits
runSummary.Ref holds a head branch for builds and an event name for soaks under one JSON key, so a consumer sees "ref": "workflow_run". watchVerdict compares State != "success" stringly when Run.Succeeded() exists, and State() returns "completed" for a done run with no conclusion, which would render as a build failure. runNext takes both out and cmd when out is cmd.OutOrStdout(). sort.Slice appears twice more in gh/runs.go. baseLabel hand-rolls strings.TrimSuffix to avoid importing strings.
Hermeticity is still open, though narrower than before: TestResolve moved off git, but fixture (resolve_test.go:84), classifyFixture.build and tagRepo are three copies of the same builder and none neutralises ambient git config. git_test.go and both classify suites are permanent and still touch real git, so commit.gpgsign=true still fails them. hack/release/next_version_test.go remains the model.
One question for the pipeline switch
next --output=github emits tag= and base= only, and TestNextGitHubOutputMatchesTheWorkflowContract pins exactly two lines. But release-prepare.yaml reads steps.bump.outputs.bump in its Push tag step and steps.bump.outputs.series in Compute tag. Deleting bump-for-branch.sh needs both keys, and next already computes them and exposes them in the JSON output. Worth deciding now whether next replaces both scripts or only next-version.sh, since the two-line assertion will resist the answer.
Review found two ways a failure produced an answer instead of an error, and the direction is what makes them different. Classify marked a release Latest when the tag query broke. ReachableTags and AllTags both returned nil, nil on any git error, so an empty candidate set reached the "nothing has shipped" branch and set Latest = true. That is what releases/latest/download resolves to, which is the install command in README.md and every guide. The tool this port absorbed refused exactly that case, and said why: An entirely empty stream means the caller's `git tag` produced nothing, which is a broken invocation rather than an answer, and answering "false" would let a release deploy on the strength of a failed command. So the port collapsed two states the original deliberately kept apart, in a change whose whole justification was fidelity. They are apart again: the tag being classified is itself a tag, so an empty result cannot be honest, and "tags exist but none is final" remains a real answer. Elsewhere a swallowed git error fails toward refusing, because ancestry catches it. Here it failed toward shipping. The swallows are gone rather than papered over. Checked the exit codes rather than assuming: `git tag --merged HEAD --list` exits 0 with empty output when nothing matches and 128 only for an unborn HEAD or a missing repository, and `git tag --list` exits 0 even with no commits at all. So there was no legitimate absence being absorbed. TagExists keeps the one real distinction, since rev-parse exits 1 for a merely missing tag and 128 for a broken one; reporting the latter as absence would let validate fall through its duplicate-tag guard. watch never ended on a failed soak. It is the soak that publishes, so a failed one leaves a draft forever, and Done was read from !Draft: the loop polled every twenty seconds until the ninety minute timeout with nothing running, then reported "gave up waiting". watchVerdict already knew the answer and never got to run. The terminating conditions are now a pure function, so they are testable without a client, and only the NEWEST soak decides - an earlier failure followed by a running retry is a recovery in progress. Mutation-tested all four, and the third attempt found a gap worth the trouble: the resolver failure tests use a fake Repo, so restoring the swallow in GitRepo changed nothing. Nothing exercised GitRepo's own error handling. TestGitRepoReportsQueryFailures does, against a directory that is not a repository, and it fails when either swallow comes back.
The Go port grew three copies of the same builder and none of them neutralised the ambient git configuration. hack/release/next_version_test.go solved this for the shell suite and said why; the port dropped the guard and reintroduced exactly the problem it describes. Reproduced before fixing. With commit.gpgsign=true set globally: fatal: failed to write commit object FAIL That is 76 resolver cases, the classify suite and hundreds of differential comparisons going red at once, on a maintainer's machine only, for a reason nothing in the output points at. One builder now, with GIT_CONFIG_GLOBAL and GIT_CONFIG_SYSTEM pointed at /dev/null and the identity supplied through the environment rather than `git config`, so a fixture needs no configuration at all and cannot inherit a template or hooks path before its first commit. Verified against a config setting commit.gpgsign, tag.gpgSign, core.hooksPath and init.templateDir together: both packages pass.
Review: cmd_status.go, cmd_preflight.go, cmd_watch.go and gh/releases.go came to 767 lines with no test file between them, and Options had no way to point a client anywhere but api.github.com. That was the reason for the split rather than a coincidence: next is the one command needing no client, and next was the one command with tests. Options gains a hidden --base-url, so the three command tests can aim at an httptest server. Hidden because it is a test seam and not a supported route to a GitHub Enterprise instance; nothing else here is written for one. Eleven cases now cover status, preflight and watch, including the two that encode release policy: a red nightly blocks, and a green one older than 48 hours blocks, because "green recently" is the actual requirement and a week-old pass describes a tree nobody has released since. The stub filters by branch, head_sha and status the way GitHub does, so the tests exercise the paths that can happen rather than ones that cannot. Separately, both commands rendered a local failure as a fact. `if err == nil` left LatestFinalTag, Live and Stale at their zero values, and the text output printed "Live trains: (none)" and dropped the stale row. "I could not tell" and "there are none" are different answers, and for the command whose whole purpose is being the dashboard the second is the worse one to guess. Ordinary causes are a stale checkout, a wrong --repo-path, or running outside a clone entirely. NextFromMain is renamed NextFromLocal, because it is resolved against whatever is checked out. On a release branch it was reporting that branch's next version under a label saying main.
All three are cases of a query that looks like it answers a question and answers a narrower one. Drafts fetched a single page and filtered it in Go, then status printed the count as though it were a total. It was really "drafts among the most recent 30 releases", and this repository already has 24 drafts in that window, so the two answers were about to diverge. Paginated, and the limit argument is gone rather than raised, because no caller wants a partial list of these. inFlight fetched ten runs per workflow and discarded the finished ones, so ten completed runs newer than a still-running one hid it entirely. That is not hypothetical: concurrent tag pushes produce exactly that. ListWorkflowRunsOptions has a Status field, so in_progress and queued are now filtered server-side. ListRuns.Limit was documented as capping how many are returned but wired to per_page with no pagination, so Limit: 200 silently became 100. Renamed for what it is and capped, so it cannot quietly return fewer results than asked for. Nothing wants the whole history; every caller wants a recent window. Also a correlation edge the design leaned on and the tests did not reach. created_at has second resolution, so two builds of the same commit within one second both failed the After test, leaving the window open-ended and merging both builds' soaks into one report. Run IDs are monotonic and now break the tie. Confirmed the case fails without it. sort.Slice replaced with slices.SortFunc while here.
…cripts Two changes that the pipeline switch depends on. --branch was policy only and never checked against the checkout. Tag discovery is separately scoped by reachability from local HEAD, so the two are independent inputs, and the default was main regardless. On a release-0.3 checkout the bare command therefore applied main's minor-bump policy to that branch's history and answered confidently. It now defaults to the checked-out branch, and warns when an explicit --branch disagrees rather than refusing, since resolving a hypothetical is legitimate and the workflow always passes one. A detached HEAD asks for --branch rather than guessing, which is what a CI checkout of a tag looks like. On a feature branch the bare command now refuses outright, which is the correct answer and was previously a confident v0.5.0. The github output emits all four keys. release-prepare reads bump= and series= from bump-for-branch.sh and tag=/base= from next-version.sh; relctl computes the policy internally in order to resolve at all, so emitting it separately would be reporting a value it already had. One call replaces both scripts and the workflow loses a step. series is written even when empty, because a missing key and an empty one differ to whatever reads it. The contract test goes from two lines to four, and caught something while doing it: the fixtures ran `git init` with the global config neutralised, so init.defaultBranch was unset and git created master. Every fixture was a branch the tool would refuse. Now explicitly main, which is what they stand in for.
Straightforward first: the mangled apostrophe from the scripted edit; cmpInt and contains replaced with cmp.Compare and slices.Contains; sort.Slice with slices.SortFunc; a local named max that shadowed the builtin; baseLabel's hand-rolled TrimSuffix; runNext taking both out and cmd when one is derived from the other. The distinct-reason floor was 15 against a table naming 19, so four reasons could have merged into an existing message and the guard written to catch that would not have. It is the number the table actually names. runSummary.Ref carried a head branch for builds and an event name for soaks under one JSON key, so a consumer saw "ref": "workflow_run". Two different facts, now two fields. While there, the verdict is carried rather than re-derived by comparing State against "success": a completed run with no conclusion renders as "completed", which that comparison would have read as a build failure. Two the port dropped rather than changed, both invisible when wrong. The shell wrote ::warning:: and ::error:: directly, which is what makes a line an annotation in the run summary. A library returning a Warnings slice should not embed them, so they are added at the point of rendering when GITHUB_ACTIONS is set. Miss this and "cutting vX.Y.Z while <train> is still in flight; that train will be stranded" simply stops appearing where a human would have seen it, and nothing fails. In the github output format they go to stderr, so they annotate without landing in $GITHUB_OUTPUT. candidateCommit's error lost the tag listing the shell included. During an incident that listing is the useful half: it says what IS there, which is usually a malformed or legacy suffix that discovery declined to treat as a candidate.
|
All of it, across six commits. Verified each finding before acting on it. The two that mattered
Fixed in
The swallows are gone rather than narrowed. I checked the exit codes independently and got your result: Mutation testing found a gap on the third attempt. Restoring the swallow in HermeticityReproduced before fixing: One builder now ( That fix then surfaced something else: with the global config neutralised, Coverage
You were right that the seam's absence was the cause rather than a coincidence. The gh layerAll three under-reporting findings fixed in
|
CI found what local runs could not: the command tests needed an ambient GitHub credential. gh.New resolves a token before the base URL is ever used, so pointing at an httptest server was not enough. They passed here because gh is logged in, and failed in CI with "no GitHub credential found" - which is the environment dependency the stub was added to remove, reintroduced one layer up. Options gains a Token seam, and Root is split so tests can build the command tree around their own options rather than reaching through flags. A seam rather than a hidden --token flag: a credential is not something to accept on a command line. Verified the way CI would: no GITHUB_TOKEN, no GH_TOKEN, and a gh on PATH that exits non-zero.
release-upgrade turns from_main and latest into job conditions, so this is the command that decides whether a cluster is touched and whether the install command in README.md gets repointed. A normal command rather than a hidden one. It answers a question worth asking before cutting anything - would this tag soak, and would it be marked Latest - and hiding it would make the pipeline's behaviour harder to reproduce when a release goes sideways. Two keys on stdout, byte-identical to what classify-release.sh emitted, so the workflow steps reading them do not change. The reasoning goes to stderr, where it annotates the run without landing in $GITHUB_OUTPUT; a test pins that separation, since a stray line on stdout corrupts a step's outputs. Checked against the shell on the real repository rather than only on fixtures: v0.4.0 and v0.3.0 agree on both keys. The test fixtures found a gap while being written: gitIn neutralised the git configuration but supplied no identity, so any fixture needing a commit rather than just a tag failed. Both helpers now share one env.
release-upgrade is one line: its resolve job already checks out the default branch with full history and has setup-go, and `go run` is what classify-release.sh did internally anyway to reach hack/cmd/semver. release-prepare needed restructuring. The old step copied hack/release/ over the working tree, which worked because the tooling was self-contained shell; relctl is Go and needs go.mod and go.sum, and overlaying main's module files onto a release branch would change the module for every other step in the job. It becomes a separate checkout under _release-tools, cone mode on hack/cmd/relctl, which brings the root files along. Confirmed with `go list -deps` that relctl imports nothing else in this repository, which is what makes that enough. Two steps become one. relctl decides what the branch may cut and resolves the version in the same pass, because it has to know the first to do the second, and emits all four keys. Push tag and the dry-run summary read bump from compute now; series has no consumer left, since the resolver applies it internally rather than handing it back. Three things the switch had to preserve, none of which the tool can do for itself: --repo-path points at the primary workspace. The branch supplies the history, main supplies the tool. Pointing it at _release-tools would resolve main's tags while reporting the branch's policy, and only a release branch would ever show it. The argument list is built with explicit ifs. `[[ cond ]] && args+=()` evaluates false when the condition is false, which under set -e exits the step - and it would fire on the most ordinary invocation there is, the one passing no optional flags. The MAJOR guard stays. bump-for-branch.sh refused anything that was not true or false; a Go bool cannot express that refusal, and a boolean input arrives here as a string, so anything unexpected would silently become false. Simulated the whole step locally before trusting it, including the no-optional-flags case, and compared against the old two-script path on the real repository: byte-identical for main, for --major, and for a release branch. MAJOR=yes still exits 1.
Three scripts, their four test files, hack/cmd/semver, and the three differential harnesses that proved the port faithful. hack/cmd/semver goes with them. Its only caller was classify-release.sh, and leaving an uncalled binary that answers semver questions would invite someone to wire it back in beside the Go implementation, which is the duplication this whole change removes. Its 27 cases came across with the is-maintenance logic. The differential harnesses go too, and that is the point rather than a loss: they were a one-time equivalence proof, and they cannot outlive the oracle they compared against. What survives is the coverage that was deliberately built to - 179 assertions in version/, 257 across relctl, none skipped - because a differential-only proof would have taken the coverage with it. Three stale references cleaned up while here: create-release-branch.yaml citing next-version.sh's pattern, and wait_rollouts_test.go's requireGit, which skipped with "skipping next-version.sh tests" in a file that has nothing to do with it. Its only callers were the deleted resolver tests, so it is gone rather than reworded.
RELEASING.md now gives each procedure twice, relctl first with the gh form immediately beneath it in a details block. Adjacent rather than in two separate sections: two procedures for one task drift, and a stale gh block is only obvious in the diff if it sits next to the thing that changed. gh is named as authoritative, which matters more than it reads. The workflows are the interface and gh dispatches them; relctl is a wrapper that also answers questions gh cannot. Where they disagree, gh is right and relctl has a bug. Version resolution is the one exception, because release-prepare calls relctl next internally, so the two agree by construction rather than by agreement. The conceptual sections are untouched. The versioning rule, semver compliance, how trains work, the tag and branch model and what gets marked Latest describe the model rather than the mechanism, and none of them changed. Break glass stays gh only, and says so: relctl deliberately does not dispatch, and making an unsoaked publish one word shorter is not a convenience worth having. Two places where relctl earns its keep rather than duplicating gh: which trains are live has no gh equivalent, because it is computed from tags rather than stored anywhere; and watch correlates runs across three workflows, only one of which names the tag. Also fixes the last stale reference to next-version-test.sh as the way to test version resolution.
cut, rc, promote, branch create, soak and publish. Dropped during two rounds of review fixes and picked back up. Every one of these can mint a tag or ship a release, and GitHub's dispatch endpoint returns 204 with no body: it does not say which run it created, and it does not report an input a workflow ignored. So the description printed before the prompt is the only chance to see what is actually being sent, and each command prints the workflow, the ref and every input before asking. cut, rc and promote also show the version they will mint, resolved locally by the same code release-prepare runs. That is the reason to prefer them over gh: the workflow's own dry_run costs a dispatch and a minute to answer a question computable here instantly. A failure to preview is reported rather than fatal, since a stale clone does not stop the workflow resolving correctly against its own checkout. They dispatch on main even when cutting from a release branch. release-prepare takes its tooling from the default branch deliberately, and dispatching it on the branch would run that branch's copy of the workflow, which is the property that step exists to prevent. The break-glass paths take a TYPED confirmation instead of accepting --yes: publish has no --yes flag at all, and soak --force-init will not take one. A script that passes --yes everywhere must not be able to reach a publish that skipped its soak. An ordinary soak retry is not break-glass and is not made to feel like one. Two things found while writing the tests. Looking for the run a dispatch created polled a 30 second timeout, which every dispatch paid and the tests paid sixty seconds of; it is ten seconds now, and the command says where to look rather than holding the terminal when a run is merely queued. And --max-notready-nodes needed Changed() rather than a zero check, because 0 disables tolerance entirely and is the opposite of leaving the flag alone. Mutation-tested the confirmation: letting --yes satisfy a typed phrase fails five cases, and dispatching before confirming fails another.
Twenty-four drafts went through joinOrNone into a tabwriter cell, so status printed them as one space-joined line that wrapped. They are one per line now. Making them vertical exposed a second problem. The order was GitHub's, passed through untouched, and it is neither semver nor date order: the list interleaved rc.9 between rc.13 and rc.8, and v0.2.0-beta.3 sat above a v0.1.24-rc.17 created two hours later. Wrapped across one line nobody notices. In a column it reads as a bug. Sorted semver-descending at collection, so the JSON is ordered too. Tags that will not parse sort by name rather than staying wherever the API left them, so the output is the same on every run. Twenty-four lines would then dominate the dashboard, so only the last 30 days are enumerated and the rest collapse to one line naming the oldest and how many there are. The header count stays the true total. Showing "Drafts (2)" while 22 sit invisible would hide exactly the ones worth cleaning up, and a backlog that stops being counted is a backlog nobody deals with. --all lists everything; -o json is never windowed, because a script asking for drafts wants all of them. The window needed a date per draft, which meant adding CreatedAt to gh.Release. PublishedAt could not do it: GitHub only sets published_at on publication, so it is the zero time for all 24 drafts here, empty for exactly the releases anything about draft age would want it for and empty without an error to say so. That is now written down on the field. A draft the API gave no date is shown rather than hidden, since a missing date is our gap and not evidence the draft is old. Mutation-tested: dropping the sort fails two, counting the window instead of the total fails one, treating undated drafts as old fails two, and windowing the JSON fails one.
status lists drafts with a COMMITTED column, and the JSON carries the same date as RFC3339. The column is headed COMMITTED because that is what it is. GitHub's created_at for a draft is the date of the tagged commit, matching git to the second on every draft in this repository, and it is the only date a draft has: published_at stays null until publication. It is NOT when the draft was made, GitHub does not expose that, and a bare date beside a tag reads as though it were. As a staleness signal for cleanup the commit date is arguably the better of the two, but the heading is what stops it being misread, so it stays. Drafts is now []draftInfo rather than []string, which retires the draftCreated side map the window needed and leaves one representation instead of two. Committed is a pointer: a draft the API gave no date is absent from the JSON rather than serialising as 0001-01-01, which reads as a real answer, and renders as an empty cell for the same reason. Nothing consumed the old string list, so the shape changed for free now rather than expensively later. The dates also make the sort order look wrong at a glance, so both comments now say what it is: highest version first, not newest first. A lower version with a later commit date sits below a higher one. Version order keeps an abandoned train together so it can go at once, which is how drafts actually get dealt with; date order would interleave series. The date column invalidated one of my own tests. It asserted each draft line carried at most one field, which a second column makes false by construction, so it now asserts one tag per line with a line count matching the draft count, which is the property the wrapped-cell bug actually violated. The line-extracting helper picks rows by a leading "v" rather than by indentation, so the new heading and the summary line are excluded structurally; the old one worked only because no test produced both at once. Nine mutations checked: rendering a zero date, dropping the heading, dropping the column, pairing each date with the next row's tag, and always setting Committed each fail; and the four from the previous commit still fail after the refactor.
IsAncestor returned `err == nil, nil`, so it could never report a failure. ae8754a gave TagExists exit-code discrimination three functions above; merge-base --is-ancestor documents the identical contract, "exit with status 0 if true, or with status 1 if not. Errors are signaled by a non-zero status that is not 1", and was left alone. The two callers fail in opposite directions, which is why this was easy to read as harmless. In the resolver a swallowed error becomes "not an ancestor", which refuses to tag: safe, and only the diagnosis was wrong, since it accused the commit of being off-branch when git had actually failed. Classify is not the same shape. FromMain=false is not a withheld claim, it is a provenance fact, and release-upgrade acts on it: the publish job proceeds when from_main is not 'true' and deploy, Orca and smoke are all skipped, which the workflow's own notice describes as publishing without a soak. A broken git command would therefore ship a release that had been deployed nowhere. Signatures and the BOM are still verified in the publish job, so it is unsoaked rather than unverified, but that is the same class as the Latest bug ae8754a fixed and in the same function. classify.go already had `if err != nil { return nil, err }` around the call. It was dead code until now. The test needed two attempts and the first was wrong in an instructive way. Injecting the failure through brokenRepo pins that Classify and Resolve propagate an error from the Repo interface, but brokenRepo embeds fakeRepo, so GitRepo.IsAncestor is never called and restoring the swallow left every test passing. The real coverage has to run git: a well-formed hash that resolves to nothing exits 128, which must be an error, while a genuine non-ancestor still exits 1 and must not be. The interface-level tests are kept as well, since they pin the callers.
Six small things, all from review. sort.Strings and sort.Slice had come back in the dispatch commit, two after 3ab0009 replaced them with the slices equivalents everywhere else. Both gone again, along with the "sort" imports. The GitHub client is built before the confirmation rather than after. Ordering only shows up when the credential lookup fails, and then it showed up at the worst moment: on a break-glass path you would type `publish v0.4.0 unsoaked` in full and only then learn there was no token, with the phrase to type again. Nothing covered this, because the test harness always injects a working token, so it comes with a test that makes the lookup fail and asserts no prompt was written. --yes is now refused by name on a typed-phrase path instead of being silently inert. It correctly never satisfied the phrase, but a script passing --yes everywhere would sit on a prompt it cannot answer and report an EOF, which says nothing about why. It still fails closed; it just says what to do instead. The existing test only passed because "y" is not the phrase, which would stay true if --yes were ignored, so it now asserts the message. branch create validates the series before dispatching. "v0.4" is the obvious thing to type when every other argument takes a tag, and learning otherwise cost a dispatch and a minute. The check is a new exported version.CheckSeries rather than a second copy of the regex: seriesShape is unexported and reaching it from package relctl is not possible, and the resolver now calls the same function, so the pattern and the message stay defined once. The RunE wrapper in soakCommand was unnecessary. The cmd passed to RunE is the command being run, so Changed() reads off it directly. The distinction it captured is kept, since --max-notready-nodes=0 disables tolerance entirely and is the opposite of leaving the flag alone. And a note on max_notready_nodes being sent as a string while force_init is sent as a bool, which reads as an inconsistency until you check the workflow: they are declared `type: string` and `type: boolean` respectively, and a dispatch whose input types disagree is rejected. Four mutations checked: ignoring --yes again, dropping the series check, swapping Changed() for a zero test, and moving the client back after the prompt each fail a test.
The content was there; the way in was not. Sections were named for the
mechanism ("From main", "From a release branch"), so finding the answer
required already knowing it: someone asking "how do I ship a fix to
v0.3.x users" has to know release branches are the answer before the
heading means anything. The two guides are now named for the question.
Cutting a major was a single sentence after the fold, offering only
`-f major=true` while `relctl cut --major` went undocumented. It has its
own subsection with both forms and a line on when to ask for one.
Cutting a patch was worse: three of the four things you need to know
before starting were in the last fifty lines of the file, under headings
you would not think to look at. That the patch never soaks, that it may
not be marked Latest, and that a branch cut from an old tag runs no CI
at all and therefore cannot merge a cherry-pick, are now stated up front
where they are load-bearing. Each links to the reference section that
explains why, so the reasoning is still in one place while the facts are
where they are needed.
That last one is also more useful than it was. Instead of "a release
predating release-* CI coverage", which requires knowing which releases
those are, it names the boundary: v0.4.0 and later are fine, v0.3.x and
earlier need release-* added to ci.yaml as the branch's first commit.
And it points out that create-release-branch already detects this and
warns, so the reader does not have to carry it at all. Verified against
the tags rather than restated: v0.4.0 contains the trigger fix, v0.3.0
and v0.2.4 do not.
"Shipping an urgent fix" and "Common situations" were two decision
guides restating three of the same cases in nearly the same words. They
are one table now, which is also the router into the guides.
The reference material is unchanged prose, regrouped: the three pipeline
phases sit under "How the pipeline works" so the ordering is carried by
the grouping rather than by numbering sections 1 to 5, two of which were
never phases at all.
Also fixed the two em-dashes the file had picked up, which AGENTS.md
disallows.
Every internal anchor was checked against the generated slugs; the three
that moved are updated and all 23 resolve.
Fourteen places had code, workflows or a shell script citing the release
document. That inverts the authority: code is what a release actually
does, and a comment that defers to prose invites the prose to be treated
as the source. It also breaks silently. Reorganising RELEASING.md in the
previous commit would have left cmd_preflight.go citing "section 1" of a
document that no longer has one, with nothing to catch it.
Four needed the substance restated rather than the citation deleted,
because the citation was carrying the reason:
- nightlyStaleAfter said RELEASING.md "asks for the nightly to be
green, and green recently, without saying how recent". It now says
what the rule is and why two days: a weekend gap in scheduling is
allowed, a week-old result is not, because that describes a tree
nobody has released since.
- the relctl package doc and the status and preflight help strings all
quoted "There is no single dashboard" to explain themselves. They
now say what they do. Quoting a document at a user reading --help
was never useful to them anyway.
Ten were redundant pointers sitting next to the thing they pointed at,
so the clause is gone and the surrounding text is unchanged: the
versioning rule in branch.go is already written out underneath it, the
rc-only rule in modes.go is already explained by the sentence it opens,
and the metalman error already prints the exact kubectl command to fix
the problem, which beats telling an operator mid-failure to go and read
a file.
Doc-to-doc links are untouched: CONTRIBUTING.md and the relctl README
still point at RELEASING.md, which is what cross-references are for.
One conflict, in the Makefile's .PHONY line, where both sides added a target to it: main gained e2e-gantry (#656) and this branch gained relctl and relctl-build. Both kept. Resolved by starting from main's line and re-applying this branch's addition, then checking that the result is exactly the union of the two sides' target sets. Taking one side's line wholesale is how a target goes missing here, and nothing would fail: .PHONY is a declaration, so a dropped entry only shows up later as a target that stops working when a file of the same name appears. Everything else merged clean. Main touched eleven files, this branch touches fifty-eight, and the Makefile is the only one in both.
Every other binary target is echoed there, forge included, and relctl was not. Noticed while resolving the merge: main's new e2e-gantry target appeared in the help output and relctl did not, which is what made the omission visible. `make help` is an explicit list rather than generated from the `##` comments, so a new target is only discoverable if it is added by hand.
Philip Lombardi (plombardi89)
left a comment
Philip Lombardi (plombardi89)
left a comment
There was a problem hiding this comment.
Doc-only pass over RELEASING.md at e2a1d564, checking every relctl claim against the code rather than reading it for prose. The guide-then-reference split works, and putting the gh form in a <details> next to each relctl form reads better than two separate sections would have.
Eleven issues. Five are inline; the rest sit on lines this PR did not touch, so they are below with citations.
Not anchorable, but the same class
RELEASING.md:664 - "each takes a typed confirmation rather than accepting --yes" is true for one of the two overrides.
publish has no --yes at all and requires the phrase. relctl soak v0.4.0 --max-notready-nodes 7, the very next example at :672, takes an ordinary Continue? [y/N] and accepts --yes. In cmd_soak.go the phrase is set only under forceInit.
That looks deliberate rather than accidental, and 39a557ad argues for it: "An ordinary soak retry is not break-glass and is not made to feel like one." But raising the NotReady ceiling is listed here as break-glass, so the doc and the code disagree about which paths are break-glass rather than merely about wording. Worth settling in one direction.
--force-init appears nowhere in this document. It is a typed-confirmation path in cmd_soak.go, a declared release-upgrade input, and it runs site init against the soak cluster instead of upgrade-apply. Break glass reads as the register of sanctioned overrides ("Two sanctioned overrides"), and this is the third, absent from it. It is also the one the code treats as most dangerous: it is what escalates soak from y/N to a typed phrase. Either it belongs beside the other two, or the section should say why it deliberately does not.
RELEASING.md:538 - bump is described as something that gets consulted, in a document that says twice that it does not exist: :145 "There is no bump input" and :345 "The bump follows from that choice and is not an input." Confirmed against the workflow: release-prepare.yaml declares mode, branch, major, pre, version, dry_run, allow_concurrent_trains, and no bump. The property the sentence states is real and worth keeping; it just needs saying without naming an input the reader has twice been told is not there.
RELEASING.md:637-656 duplicates :32-44 rather than continuing from it. :34 and :639 are the same sentence, and the make relctl-build block at :40 is repeated at :653. "Every procedure below" is also wrong at :639: that is line 639 of 764, and everything it refers to is above. The structure the doc sets up is right, and :44 states it ("The full account ... is in [relctl and gh]"), so this section should open with the account. The genuinely new material is the authority exception and the README link, and both are currently preceded by two paragraphs already read.
Duplication that has already drifted
:205and:464are the same paragraph about the nightly not covering release branches, with--branch release-0.3in one and--branch release-0.4in the other. Nothing keeps them in step, and they have not stayed in step.:79-84and:443-447repeat the nightly and CIghblock verbatim, comments included, though:88already links to the reference section for what the checks mean. Plausibly deliberate so the walkthrough stands alone, so I have not called it a defect.
The doc elsewhere prefers a link to a repeat, which is why these stand out rather than being a general style point.
Scope
All of this is prose against behaviour that is already correct, with one exception: the comment on :265 asks for a guard in cmd_classify.go, because the doc problem there is a symptom rather than the cause.
version.Classify's doc comment says it expects a checkout of the default branch and the command's help repeats it, and nothing checked. Run from release-0.4, the tag v0.4.1 is reachable from HEAD, so from_main comes back true for a release that was not cut from main, and latest is computed against that branch's trunk instead of main's. Both answers wrong, and neither able to fail, because there is nothing for them to be inconsistent with. RELEASING.md walked readers into exactly that: it had them check the release branch out at step 2 and run classify at step 4, with a trailing comment as the only guard. The document is fixed separately; the guard belongs in the command, which also covers running it outside that procedure. A refusal rather than a warning, and next deliberately keeps its warning. The two are not the same case. next takes an explicit --branch, so a mismatch means policy and history disagree, and it says which one it resolved against; more to the point its answer is still structurally guarded, because a version computed from the wrong history either falls outside the requested series or collides with an existing tag. Checked rather than assumed: `next --branch release-0.3` from a main checkout exits 1 with "computed tag v0.4.1 is outside series 0.3" and writes nothing to stdout. classify has neither a flag nor a guard, there is no reading of "reachable from HEAD" that is merely different rather than false, and --output json and github put the answer on stdout where a warning on stderr would not be seen. A detached HEAD warns instead of refusing. CurrentBranch already reports empty rather than guessing, and refusing on "cannot tell" would break a legitimate checkout of a commit to defend against a case that cannot be detected. It is also the safe direction for CI: release-upgrade checks out ref: default_branch, which yields a real branch, so the guard is silent there, and were it ever detached it would warn rather than break the pipeline. Three mutations checked: dropping the guard fails three tests, refusing on detached HEAD fails one, and warning instead of refusing fails two.
A doc-only review checked every relctl claim against the implementation rather than reading for prose. Most of what it found I introduced in e30db7d, including the worst one. `relctl branch create --dry-run` was documented as resolving the branch point and warning about missing release-* CI triggers, under "You will be warned rather than having to remember". It does neither. branchCommand builds a static summary and dispatch.run returns before the client is built, so nothing is resolved and nothing is fetched. The workflow resolves the branch point, which is why its own dry_run=true reports it and has to dispatch to do so. The document draws exactly that distinction sixty lines earlier and I contradicted it, then told the reader they could stop remembering something they still have to remember. Both places now say which of the two produces the answer. Break glass listed two overrides and claimed each takes a typed confirmation. There are three, and they take three different confirmations: raising the NotReady ceiling is an ordinary y/N prompt, force_init and force_publish need a typed phrase. force_init was absent entirely despite being the one the code guards hardest, and it runs `site init` against the soak cluster, which on an already-initialised cluster creates a fresh Site rather than migrating the existing one. It now has a section, and the three sit in a table that says what each bypasses and what it asks for, so the doc and the code can be compared at a glance instead of by reading prose. The patch procedure told the reader to check out release-0.3 and then run classify, which answers relative to the trunk. That produced two silently wrong answers; the guard added in 9ea8154 now refuses it, and the procedure checks main out first. The step above it demonstrated the `--branch` default by passing `--branch` explicitly, which showed the opposite of what its comment said. The claim that `next` "warns when the two disagree" undersold it: for the pairing named in that same sentence it refuses, because the computed tag falls outside the series. Checked rather than assumed. Two duplications, one of which had already drifted: the nightly paragraph existed twice with different branch numbers, and the relctl/gh section repeated two paragraphs from the top of the file while saying "every procedure below" from line 639 of 764. The reference copy now carries only what is new. That drift is the risk we accepted when inlining facts into the guides, and it arrived in the same commit that introduced it, which is worth recording. Also: one procedure used two different versions across its relctl and gh halves; :350 was the last place in the repository calling the release tooling "scripts" after 28e5556 deleted them; and How trains work described `bump` being consulted, an input the document twice says does not exist and release-prepare does not declare. Verified by extracting every relctl invocation from the document and checking each command and flag against the built binary's help, and by re-running the anchor check: 28 links, all resolving.
|
All eleven addressed, plus a twelfth the review did not catch. Two commits: The
I deliberately did not give Break glass is a table of three. Your point that the doc and code disagreed about which paths are break-glass was the right framing. A twelfth, surfaced by a question about whether On the duplication. Rest as described: Left alone deliberately: the repeated Verified by extracting every |
Philip Lombardi (plombardi89)
changed the title
2 participants
Problem
Release state has no single place to look.
RELEASING.mdsaid so itself: "There is no single dashboard." Whethermainis releasable, which candidate trains are live, whether a release will be marked Latest, where one has got to across three workflows - each of those needed a differentghinvocation, or could not be answered at all.The versioning rule deciding all of it lived in ~500 lines of shell that only CI ever called. Building a CLI on top of that would have put the rule in two places, and the two would drift.
Solution
The rule moves into Go and the pipelines call that, so humans and CI share one implementation instead of agreeing by convention.
relctlis the result.status,next,preflight,classifyandwatchanswer questions about a release;cut,rc,promote,branch create,soakandpublishdrive one. The shell it replaces is deleted rather than left alongside.ghstays authoritative: the workflows are the interface andrelctlis a convenience over them. The one exception is version resolution, whererelease-prepareinvokesrelctl nextitself, so the tool and the pipeline agree by construction rather than by review.RELEASING.mdis reorganised around the two questions people actually arrive with - how to cut a minor or major, and how to cut a patch for a series that already shipped - with the reference material behind them.