ci: read the Go version from go.mod instead of six literal pins - #297
Conversation
miguel-heygen
left a comment
There was a problem hiding this comment.
Exact-head review at 28f146f.\n\nThe current workflow conversion works: all six setup-go steps point at go.mod, exact-head CI resolves 1.25.13, exports GOTOOLCHAIN=local, and every reported check is green.\n\nblocker — cmd/heygen/go_version_pin_test.go:22-23 deliberately checks only workflow files and never reads go.mod, so the central invariant is not actually enforced. Two known edits make the test stay green while changing the release toolchain contract:\n- changing the directive to go 1.26 makes setup-go float to the latest 1.26 patch instead of preserving the exact-patch behavior this PR replaces;\n- adding a toolchain directive makes setup-go prefer that value, so the go directive is no longer the single source even though TestWorkflowsTakeGoVersionFromGoMod still passes.\n\nPlease have the test parse go.mod and require the chosen policy explicitly: for the current reproducible-pin contract, require a three-component go directive and reject a toolchain directive. If the team instead chooses floating patches, encode and document that policy rather than leaving it implicit. Mutation-pin both cases.\n\nimportant — RELEASE.md:76-78 documents that GoReleaser builds and publishes the artifacts but does not say the release toolchain now comes from go.mod. Repository AGENTS.md requires RELEASE.md updates for workflow changes affecting how releases are built. Add the source-of-truth and patch-policy rule there so the operator editing go.mod sees the release consequence.\n\nVerdict: REQUEST CHANGES\nReasoning: The current head behaves correctly, but the new guard passes under both known ways to violate its stated single-source/exact-pin contract; that is the drift this PR exists to prevent.\n\n— Magi
|
Addressed both findings at blocker (test did not enforce the invariant). Confirmed and fixed.
The A third assertion guards a missing Policy, previously left implicit. Resolved as a reproducible pin: releases ship checksummed binaries, so rebuilding a tag must produce the same one. Encoded in the test rather than documented and hoped for. Note the three-component rule also rejects an exact prerelease pin like important (RELEASE.md). You were right and I had this wrong. I read AGENTS.md:85 ("triggers, versioning, channels") and concluded nothing was owed, missing line 88: "CI workflow changes that affect how releases are built, triggered, or distributed count as release workflow changes." Where the release toolchain comes from is squarely that. RELEASE.md now has a Two boundaries recorded in the test comment rather than guarded: a reusable workflow or composite action installing Go without a visible |
miguel-heygen
left a comment
There was a problem hiding this comment.
Exact-head re-review at 025bacf.\n\nBoth prior findings are resolved:\n\n- cmd/heygen/go_version_pin_test.go now reads go.mod itself, rejects any toolchain directive, and requires the go directive to contain major.minor.patch. The two bypasses are pinned directly: go 1.26 fails the patch-component rule, and toolchain go1.24.0 fails because setup-go would prefer it before exporting GOTOOLCHAIN=local.\n- RELEASE.md now documents go.mod as the only release-toolchain source, explains exact-patch reproducibility, and tells operators to remove toolchain directives and bump go instead.\n\nThe original six-workflow sweep remains intact, every exact-head CI job is green across Linux, macOS, and Windows, and govulncheck still passes with only the existing documented allowlist. No new findings.\n\nVerdict: APPROVE\nReasoning: The central single-source/exact-pin contract is now enforced at both workflow and go.mod boundaries, mutation-covered, documented, and green at the exact head.\n\n— Magi
The same version string was written in seven places: the go directive in go.mod, four setup-go pins in ci.yml, and one each in release-stable.yml and dev-release.yml. Nothing kept them in agreement except remembering to grep, and the previous patch bump had to touch all six workflow spots by hand. That drift is not cosmetic. setup-go exports GOTOOLCHAIN=local, so the toolchain it installs cannot upgrade itself to satisfy go.mod: a pin that falls behind the go directive fails the build outright, and one that runs ahead of it builds against a newer stdlib than the module declares. Every setup-go step now reads go-version-file: go.mod, leaving the directive as the only place the version exists. Adds a test over the workflow files asserting no step reintroduces a literal go-version, since the whole benefit disappears the first time someone re-pins one by hand. It fails if a literal pin appears, if go-version-file names anything other than go.mod, and if the sweep matches no setup-go steps at all — that last case because an empty match set would otherwise pass while checking nothing. It reads both .yml and .yaml, since this repo already spells one config .goreleaser.yaml and a .yaml workflow would otherwise land outside the sweep. Verified setup-go resolves the directive as intended by replaying its own parser (installer.ts parseGoVersionFile) against this go.mod: no toolchain directive present, go directive 1.25.13, resolved 1.25.13. Resolution is exact rather than floating only because the directive carries a patch component — setup-go matches its input with semver.satisfies, under which "1.25.13" admits only that version while a two-component "1.26" admits any 1.26.x. Behaviour is therefore unchanged from the literal pins this replaces; whether the repo wants to guarantee that by requiring a patch component is a policy question left open here. Note for whoever bumps next: the two release workflows are workflow_dispatch only, so no automatic run exercises them. This PR's CI covers the four ci.yml jobs; the release paths are covered by the test above rather than by execution. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review of the previous commit found the guard did not actually enforce what the change claims. It read the workflow files and never opened go.mod, so both ways of breaking the single-source contract left it green. Two edits did that. A two-component `go 1.26` directive turns setup-go's exact match into a range, so every job takes the latest patch and a rebuilt tag can link a different stdlib. Adding a `toolchain` directive is worse: setup-go prefers it over the `go` directive whenever GOTOOLCHAIN is not already "local", and it is not, because setup-go resolves the version before exporting that variable. A `toolchain go1.24.0` line would have silently reinstated the stdlib the parent commit exists to remove, with go.mod still reading 1.25.13. Settles the question the previous commit left open: this repo wants a reproducible pin, not floating patches, because releases ship checksummed binaries and rebuilding a tag must produce the same one. TestGoModPinsAnExactToolchain requires a three-component directive and rejects a toolchain directive. Both were mutation-pinned. A missing `go` directive is guarded too, though only defensively: dropping it makes the module unbuildable long before any test runs. Documents the rule in RELEASE.md, which AGENTS.md requires for a change to how releases are built, and the toolchain source is exactly that. The previous commit judged no doc update was owed by reading only the "triggers, versioning, channels" line and missing the broader clause below it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
025bacf to
b2911e8
Compare
Description
Stacked on #296. That PR bumped the Go toolchain by hand-editing the same version string in six
workflow spots; this one removes the need to ever do that again.
The version was written in seven places: the
godirective ingo.mod, foursetup-gopins inci.yml, and one each inrelease-stable.ymlanddev-release.yml. Nothing kept them in agreementexcept remembering to grep.
That drift is not cosmetic, and the reason is worth knowing because it is not obvious from reading the
workflows.
setup-goexportsGOTOOLCHAIN=localitself:With
GOTOOLCHAIN=local, the installed toolchain cannot upgrade itself to satisfygo.mod. So a pinthat falls behind the
godirective fails the build outright, and one that runs ahead of itbuilds against a newer stdlib than the module declares. Every
setup-gostep now usesgo-version-file: go.mod, leaving the directive as the only place the version exists.How it works
setup-goreads the version out ofgo.moditself. Its parser prefers atoolchaindirective, butonly when
GOTOOLCHAINis notlocal, then falls back to thegodirective:Our
go.modhas notoolchainline, so both paths land on thegodirective and resolve1.25.13.One property to understand before editing
go.mod, because it is silent:setup-gomatches its inputwith
semver.satisfies, so whether the toolchain is pinned or floating depends on how manycomponents the directive has.
go 1.25.13go 1.26Today the directive is patchful, so resolution is exact and behaviour is identical to the literal
pins this replaces. If someone later writes a two-component directive, every job silently starts
taking the latest patch. That trade is real in both directions: floating picks up stdlib security
fixes without a workflow edit (and would have pre-empted #296 entirely), while pinning keeps release
builds reproducible. This PR does not decide it, and deliberately changes no behaviour; it is recorded
here so the choice is made knowingly rather than discovered.
Design decisions
go-version-filerather than a workflow-level env var or a reusable workflow. An env var wouldstill be a second copy of the number, just a better-organised one. Reading
go.modmakes the module'sdeclared minimum and the installed toolchain the same value by construction, so they cannot disagree.
A test, not just a convention. The entire benefit evaporates the first time someone re-pins a
literal version by hand, and a reviewer would have to notice.
cmd/heygen/go_version_pin_test.goasserts the invariant instead. It follows the precedent of
generated_example_flags_test.go, whichsimilarly reads a repo file outside its own package to guard a cross-file invariant.
The test reads both
.ymland.yaml. This repo already spells one config.goreleaser.yaml, soa
.yamlworkflow was a live way to land outside the sweep. Its documented boundary is directsetup-gosteps in top-level workflow files; a reusable workflow or composite action could install Gowithout a step it sees. Nothing uses either today, and the comment says so rather than leaving it
implied.
Testing
reverted.
go-version: "1.25.14"inci.yml→ fails, naming file and job.go-version-fileat.go-version→ fails on the mismatch.setup-gosteps matched → trips the explicitsteps == 0guard,which exists because an empty match set would otherwise pass while checking nothing.
.yamlworkflow carrying a literal pin → fails, naming that file. This is the bypass theextension fix closes, so it was verified specifically rather than assumed.
setup-gosteps across the 3 workflows usego-version-file: go.modwith noliteral
go-versionleft, by parsing each file with a YAML parser and asserting per step.semverlibrarysetup-gouses,which is where the pinned-vs-floating table above comes from.
make testandmake lintboth clean. No new dependency:gopkg.in/yaml.v3was already direct.The four
ci.ymljobs on this PR exercise the change directly. The two release workflows areworkflow_dispatchonly, so no automatic run covers them; the test stands in for execution there,which is called out in the commit message for whoever bumps next.