Conversation
|
@claude-fleet review |
|
Review task queued. Task: 32828517-04b8-469e-b8f7-74f135bba9a4 |
There was a problem hiding this comment.
Verdict: request changes — two one-line fixes. The Go code is correct; I cross-checked internal/buildinfo against the real toolchain and it agrees on every case I could construct.
What I verified holds
I built a throwaway main that calls buildinfo.Read() and compared its answer against what go build embeds, in a local clone of this repo with synthetic tags (go1.26.5, linux/arm64):
| case | toolchain (go build in a clone) |
versionFromDescribe input |
agrees |
|---|---|---|---|
| at a tag | v0.9.9 |
v0.9.9-0-g9368c0e |
yes |
| after a tag | v0.9.10-0.20260916203853-6adda9de3827 |
v0.9.9-1-g6adda9d |
yes |
| after a prerelease tag | v0.9.11-rc1.0.20260916203859-1569a6f9798a |
v0.9.11-rc1-1-g1569a6f |
yes |
So the patch bump, the -0. versus .0. split on prerelease tags, and the 12-hex/UTC-timestamp suffix all match what the toolchain would have derived. The unit-test table in buildinfo_test.go is not self-referential — its expected strings are the toolchain's.
The worktree premise is real on go1.26.5, not folklore: git worktree add --detach then go build gives dev (go1.26.5, linux/arm64) with no vcs.* settings at all. Building the same tree in that worktree with the Makefile's -X flags reproduces the clone's answer byte for byte (v0.9.11-rc1.0.20260916203859-1569a6f9798a (1569a6f, …)), so the fallback does what internal/buildinfo's package comment claims and only what it claims — withGit leaves a toolchain-derived version alone, which the tests pin.
Two other things I checked rather than took on faith:
-Xwith an empty value links fine, which is the common case forgitDescribein a clone with no tags fetched. No build failure there.- Untracked files alone make Go stamp
+dirty(this workspace's untrackednode_modulesproduced…+dirty), so the.goreleaser.yamlandrelease.ymlcomments about a tidy hook dirtying the release are describing a real mechanism.
cli.VersionPrinter in urfave/cli v3.10.1 is a package var of func(*Command) that ShowVersion calls from command_run.go:222, reached when cmd.parent == nil && !cmd.HideVersion. The init() in version.go therefore does capture hull --version, and since Version: build.String() is never empty the flag is no longer hidden — which is the flag provided but not defined: -version failure the description opens with.
The VERSION retirement is complete: after this diff, grep -rn VERSION over Makefile, workflows, scripts, packaging and docs turns up no reader of the file. Nothing else stamps main.version either.
Gates I ran
go test ./internal/buildinfo/—ok, 0.009s.GOOS=darwin GOARCH=arm64 go vet ./cmd/hull/ ./internal/buildinfo/— clean. This is the only way I could exercise thecmd/hullside; those files are//go:build darwinand this workspace is linux, so the version tests never ran.make -pto read the computed variables, which is where theRELEASE_VERSIONfinding below comes from.
I restored the tree after each experiment; git status --porcelain shows only the pre-existing untracked node_modules.
Findings
Makefile:88 — the || echo 0.0.0 fallback can never fire. The || binds to the whole pipeline, and the pipeline's status is sed's, which is 0 even when git describe printed nothing. make -p in a clone without tags prints:
RELEASE_VERSION :=
TARBALL := …/dist/hull--arm64.tar.gz
The old cat VERSION || echo 0.0.0 worked because cat was last in the pipeline. Blast radius is small — no workflow runs make tarball, so this only hits the local dry run — but the guard is dead as written.
docs/build.md:140 — the heading and its first paragraph now contradict the rest of the section. The heading still says --version is absent from a plain go build, and the paragraph under it still says the version arrives through an -ldflags -X that "the Makefile and goreleaser supply". After this PR goreleaser supplies no version flag at all and the Makefile's -X carries git facts rather than a version string, and the next paragraph says so. This is the section the PR edited, so the stale half survives directly above its own correction.
The rest are nits and do not need another round: printVersion(nil) at version.go:62 re-reads the build info the Action just read into info; the strings.Contains(out, " (") assert at version_test.go:45 cannot fail independently of the equality check above it, given String() always emits " ("; and git diff --exit-code at release.yml:92 misses untracked and staged changes, where git status --porcelain is exactly the command Go's dirty flag keys on.
Happy to approve once the two above are in.
Not checked:
- cmd/hull tests were not executed: version.go and version_test.go are //go:build darwin and this workspace is linux/arm64. I type-checked them with GOOS=darwin GOARCH=arm64 go vet, but did not run TestVersionPrintsTheBuild, TestBothVersionSpellingsAgree or TestVersionJSONCarriesTheBuild, and did not prove they fail with the init() removed.
- The brig-sh/brig#231 original this is ported from is not reachable from here, so I could not compare internal/buildinfo against the brig package it is said to be read from verbatim, nor confirm the claim that hull needs nothing from the doctor/daemon parts of that stack.
- The hvi-vmm submodule is not checked out in this workspace, so I could not confirm whether
make vz_runner hvi_vmmleaves untracked content inside it that would show as a dirty submodule in the parent'sgit status --porcelainand stamp the goreleaser-built hull as +dirty. - goreleaser's internal pipe ordering — whether its own dirty-tree validation really runs before
before.hooks, as the new .goreleaser.yaml and release.yml comments assert. The mitigation is sound either way, since Go re-reads git state at build time. - No release was actually cut, so the end-to-end claim that a tagged release reports the bare tag on the self-hosted runner is unverified; I only reproduced it in a synthetic local clone.
- .github/workflows/ci.yml beyond the one comment line the diff touches, and the docs/releasing.md renumbering beyond reading it.
Task 32828517-04b8-469e-b8f7-74f135bba9a4 on rp2, model claude-opus-5, effort medium.
| RELEASE_VERSION := $(shell cat $(CURDIR)/VERSION 2>/dev/null || echo 0.0.0) | ||
| # The tag is the version. `make release` is a local dry run of what goreleaser | ||
| # does on a tag, and goreleaser takes its version from the tag too. | ||
| RELEASE_VERSION := $(shell git describe --tags --abbrev=0 --match 'v[0-9]*' 2>/dev/null | sed 's/^v//' || echo 0.0.0) |
There was a problem hiding this comment.
should_fix: The || echo 0.0.0 fallback can never fire. || binds to the whole pipeline, and the pipeline's exit status is sed's, which is 0 even when git describe failed and printed nothing. In a clone with no tags reachable, make -p prints RELEASE_VERSION := (empty) and TARBALL := .../dist/hull--arm64.tar.gz — verified in this workspace. The previous cat VERSION 2>/dev/null || echo 0.0.0 worked because cat was the last command in the pipeline. Blast radius is limited to the local make tarball/make release dry run (no workflow uses TARBALL; the release job's archive check globs goreleaser's output), but the guard is dead code as written. Moving the fallback inside the substitution, e.g. $(shell v=$$(git describe --tags --abbrev=0 --match 'v[0-9]*' 2>/dev/null); echo "$${v:-v0.0.0}" | sed 's/^v//'), restores it.
| @@ -140,14 +140,13 @@ A **rebuild** does invalidate a signature, because it produces different bytes. | |||
| ## `--version` is absent from a plain `go build` | |||
There was a problem hiding this comment.
should_fix: The heading and the paragraph under it are false after this PR, and contradict the paragraph immediately below them. The heading still reads --version is absent from a plain go build, while the section now explains that it is present. The first paragraph still says the version string "arrives through an -ldflags -X, which the Makefile and goreleaser supply": .goreleaser.yaml in this same diff drops -X main.version={{.Version}} and supplies no version flag at all, and the Makefile's new GITFLAGS pass git's commit/time/describe/modified, not a version string. A reader debugging where a release's version comes from is sent to look for an -X that no longer exists. This is the one doc section the PR rewrote, so the stale statement survives directly above its own correction.
| if cmd.Bool("json") { | ||
| return printJSON(os.Stdout, versionData(info)) | ||
| } | ||
| printVersion(nil) |
There was a problem hiding this comment.
nit: The Action reads info := buildinfo.Read() on line 57 and then, on the non-JSON path, calls printVersion(nil), which calls buildinfo.Read() again. Harmless today because Read() is deterministic, but it means the human line and the JSON payload in one command are built from two separate reads. Passing info to the printer (and having the init() wrapper do the read) would make the single-source property structural rather than incidental — which is the same argument the file's own comment makes for installing the printer beside the function.
| [ "$(cat VERSION)" = "$TAG" ] || { | ||
| echo "VERSION file ($(cat VERSION)) does not match tag $TAG" >&2; exit 1; } | ||
| go mod tidy | ||
| git diff --exit-code |
There was a problem hiding this comment.
nit: git diff --exit-code compares the worktree against the index only: it misses untracked files and anything already staged. Go's vcs.modified keys on git status --porcelain, whose output includes untracked entries — confirmed in this workspace, where an untracked node_modules alone is enough to make a build report +dirty. go mod tidy only rewrites tracked go.mod/go.sum, so the check does cover its stated case; using git status --porcelain (and failing on any output) would make the check test the same condition the binary will later report, rather than a subset of it.
| if out != want { | ||
| t.Fatalf("hull version printed %q, want %q", out, want) | ||
| } | ||
| if !strings.HasPrefix(out, "hull ") || !strings.Contains(out, " (") { |
There was a problem hiding this comment.
nit: This assert cannot fail independently of the equality check four lines above it. want is built as "hull " + buildinfo.Read().String() + "\n", and Info.String() unconditionally emits fmt.Sprintf("%s (%s)", ...), so any out that passed the out != want check already has the hull prefix and a " (". It reads as a format guard but pins nothing extra. Asserting against a literal expected line built from a fixed Info would pin the format for real.
`hull --version` printed the string the Makefile or goreleaser stamped into `main.version`, and a bare `go build` stamped nothing, so the flag was hidden entirely. Neither form said which build it was: two binaries from one tag, or a tag rebuilt after a force-push, printed the same line, and a tree with uncommitted changes printed what a clean one did. The binary already carries the answer. Go records the module version it derives from the nearest reachable tag, the commit, its time and whether the tree was modified. internal/buildinfo reads those, so nothing is stamped at link time any more. `hull version` is new and takes --json; `hull --version` prints the same line through the same printer, installed beside it rather than in main so the two cannot come apart. Telemetry reports the derived version in place of the stamped one. The one case the toolchain cannot answer is a linked git worktree: its .git is a file, which the toolchain does not read as a repository, so it embeds nothing. make passes git's own commit, time, nearest tag and modified flag, and the binary uses them only when the toolchain embedded none. goreleaser passes none. The VERSION file goes with the stamp. It existed so a reader had a version to look at, and release.yml asserted it matched the tag so it could not drift; now that the binary reports the tag the toolchain derived, the file is a second copy of the same fact with nothing reading it. The release workflow gains the tidy check in its place: goreleaser reads the tree before its hooks run, so a `go mod tidy` hook that touched go.sum would build every binary as +dirty with nothing failing. Ported from brig-sh/brig#231, which is the same change on that side. Verified both paths: from a clone the toolchain gives v0.1.0-rc27.0.20260916191606-8a431dc1aaae with the commit and its time, and in a worktree make's fallback gives the same shape with +dirty. Signed-off-by: Anastassios Nanos <ananos@nofire.ai>
f95ba4e to
57db905
Compare
|
All five in, the two must-fix and the three nits. Thank you for the cross-check against the real toolchain -- the synthetic-tag table is the part I could not do from here, and it is the part that matters most in that package.
RELEASE_TAG := $(shell git describe --tags --abbrev=0 --match 'v[0-9]*' 2>/dev/null)
RELEASE_VERSION := $(or $(patsubst v%,%,$(RELEASE_TAG)),0.0.0)Checked both ways with
The three nits, all taken. On one thing you could not check: I did prove the tests fail with the The other gaps you listed are fair and I am not claiming otherwise: no release was cut, so a tagged build reporting the bare tag on the notary runner is still unverified, and goreleaser's pipe ordering is asserted from its docs rather than read. Both are things the first release after this lands will settle.
|
Summary
hull --versionprinted the string the Makefile or goreleaser stamped intomain.version, and a barego buildstamped nothing at all -- the CLI libraryhides the flag when the string is empty, so a hand-built binary answered
flag provided but not defined: -version.Neither form said which build it was. Two binaries from one tag, or a tag
rebuilt after a force-push, printed the same line, and a tree with uncommitted
changes printed what a clean one did. The rc series ships many builds per
version, so "which 0.1.0-rc27" is a question a bug report could not answer.
The binary already carries it. Go records the module version it derives from
the nearest reachable tag, the commit, its time, and whether the tree was
modified.
internal/buildinforeads those and nothing is stamped any more.This is brig-sh/brig#231 ported across -- same package, same reasoning, same
retirement of the VERSION file.
Related issues
Ports brig-sh/brig#231 (refs brig-sh/brig#230).
Changes
internal/buildinfo, read from brig verbatim: the version, commit, committime, modified flag and platform the toolchain embedded.
hull version, with--json.hull --versionprints the same line throughthe same printer, installed beside it rather than in
mainso the twospellings cannot come apart by one being forgotten.
makestops stampingmain.versionand passes git's commit, time, nearesttag and modified flag instead. The binary reads those only when the
toolchain embedded nothing, which is a build in a linked git worktree --
there
.gitis a file and the toolchain does not read it as a repository.goreleaser passes none.
VERSIONis retired, with the tag-matches-VERSION check inrelease.yml.The file existed so a reader had a version to look at and the check kept it
from drifting; the binary now reports the tag the toolchain derived, so the
file is a second copy of the same fact with nothing reading it.
release.ymlgains the tidy check in its place. goreleaser reads the treebefore its hooks run, so a
go mod tidyhook touchinggo.sumwould buildevery binary as
+dirtyand nothing would fail.docs/releasing.md(the bump step becomes a tag) anddocs/build.md(a barego buildnow reports a real version).Checklist
make testpasses-- Go only;make macosbuilds hull, vz-runner and hvi, if Go, Swift or Rust code changedmake urunc_macoswas run and is how the worktree fallback below was checkedI have run the e2e harnesses (-- nothing in the boot, console or run path changedtest/*.py)docs/)Both paths were checked, because they fail differently and only one of them is
reachable from a worktree:
--jsoncarries the commit in full, and leavescommitandcommitTimeoutrather than empty when a build has no VCS data, so a consumer tests for the key
instead of recognising a zero time.
Notes for review
Two things worth a second opinion.
The version printer is installed in an
init(). I would normally avoidone, but the alternative is an assignment in
mainthat a test cannot reach --which is exactly how the first version of this let
hull versionandhull --versionprint two different lines. Installing it beside the function itbelongs to removes the failure mode; there is a test asserting the two agree.
What is not ported. brig's stack also taught
brig doctorto name the brigand brigd builds, and had brigd report the build over its protocol. hull has
neither a doctor nor a daemon, so there is nothing to carry across.
One follow-on: #75 composes a channel version from
cat VERSIONplus the date and the short SHA. Once this lands it should read the version off
the binary the way brig-sh/brig#229 now does, and the three lines that do it
there drop straight in.