feat(cli): report the build behind the binary in brig version - #231
Conversation
|
@claude-fleet review |
|
Review task queued. Task: 3940b0fd-43b2-4142-916c-838caa41b0aa |
![claude-fleet[bot]](https://avatars.githubusercontent.com/in/4876296?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict below. I reviewed the diff at 13e9bb7 against merge-base ef4aa8b, full history fetched.
Verdict: approve
Clean, well-scoped change. It does what the description claims: the -X main.version stamp and the VERSION file are gone, and both binaries now derive version/commit/time/modified/platform from debug.ReadBuildInfo. The code is careful about the absent-vs-empty distinction (commit/commitTime omitted, not zero-valued), and the tests are thorough and non-tautological.
What I verified holds
internal/buildinfo:Frommaps(devel)and nil build info todev, keeps a real tag/pseudo-version verbatim (including+dirty), falls back toruntimefor Go version/OS/arch so no field is empty.String()drops commit/date when absent. Tests cover tagged, pseudo-version+dirty, proxy (no commit), nil, and UTC date rollover — all pass.brig version/--version:versionCmdaccepts--jsonin either flag position and after either spelling, refuses any other trailing word (version_test.go,version_json_test.go). Envelope isbrig.sh/v1alpha1kindVersion,--jsonregistered inverbTakesGlobalJSONandjsonUnsupportedf. Global-help and error strings updated to listversion.- brigd
versionop:versionResponseaddscommit/commitTime/modifiedbesideversion,omitemptyon all so an old daemon and a no-VCS build both send absence rather than zeros. Startup stderr line usesbuildinfo.Read().cmd/brigd/version_test.goconfirms the wire fields match the local build. askBrigdBuild: 2s dial + 2s deadline, tolerates a response without trailing newline (len(line)==0guard), treats a non-okor unparseable reply as an error the caller downgrades topass. brigd replies viajson.Encoder.Encode, which terminates with\n, so the read does not block to the deadline.brig doctor: opens with thebrigrow (informational, first), and asks a running daemon its build; mismatch on version-or-commit is!!with a restart fix.doctor_build_test.goexercises same/different/no-answer via a faithful fake (0600 socket, held lock, pid sidecar). smoke.sh asserts the first doctor row equalsbrig version.- CI/build:
release.ymltag-vs-VERSION check removed with the file;.goreleaser.yamlandMakefiledrop-X main.versionand document why full-clone + clean-tree matter. No staleVERSION/main.versionreferences remain (theBRIG_VERSIONinstall var and hull version are unrelated). - docs/cli.md, docs/brigd.md match the implemented output, flags, and envelope.
Gates
go build ./...— okgo vet ./cmd/... ./internal/buildinfo/...— okgo test ./cmd/brig/... ./cmd/brigd/... ./internal/buildinfo/...— ok (all three packages pass)
I did not run full make all, -race, or script/smoke.sh.
One nit
doctor.go compares only Version and Commit, not Modified. For release builds Go folds dirtiness into Version via +dirty, so it is caught there. But for local go build/make (devel) builds, a dirty brigd running against a clean brig at the same commit both read version dev with the same commit, so the comparison reports a match while the daemon actually carries uncommitted changes brig does not. Narrow, dev-only, and fail-safe in direction; adding || theirs.Modified != mine.Modified would close it.
Not checked:
- full
make all,go test ./... -race, and script/smoke.sh (340 checks) were not run - per-commit
builds/vets/passesclaim was not verified commit-by-commit - goreleaser and release.yml were read but not executed
Task 3940b0fd-43b2-4142-916c-838caa41b0aa on ci-16gb-fsn1-1, model executor default, effort high.
| theirs, err := askBrigdBuild(socket) | ||
| if err != nil { | ||
| return check{Name: "brigd", State: statePass, | ||
| Finding: fmt.Sprintf("%s, did not answer a version request: %v", finding, err)} |
There was a problem hiding this comment.
nit: The brigd build comparison checks Version and Commit but not Modified. For release builds dirtiness rides in Version via +dirty, so it is caught. For local devel builds (go build/make) a dirty brigd against a clean brig at the same commit both read dev + same commit and compare equal, so doctor reports a matching build while the daemon actually carries uncommitted changes. Narrow and fail-safe; || theirs.Modified != mine.Modified closes it.
|
Just curious, I am surprised goreleaser doesn't have that functionality OOTB. Did we check whether that flow is supported? |
ananos
left a comment
ananos
left a comment
There was a problem hiding this comment.
Thanks @asapranidis, this is a nice cleanup. Dropping the VERSION file and letting the toolchain answer the question is the right call, and the docs/releasing.md rewrite is useful on its own -- the pseudo-version table and the maintenance-branch diagram are both correct against Go's actual rules.
I went through the 7 commits in order and checked the behaviour end to end. Everything the PR body claims holds:
go build ./...,go vet ./...andgo test ./...are green, and each of the 7 commits builds, vets and tests standalone.script/smoke.shpasses (exit 0), including the new doctor assertion.- A tagged build prints the bare tag, a build after a tag prints the pseudo-version, and a dirty tree appends
+dirtyand sets"modified": true. - brigd's
versionop answers with the new fields, and doctor marks a mismatched daemon!!with the restart fix.
The ordering in brigdCheck is good: the 0600 mode check returns before anything dials, so a socket with the wrong mode is never spoken to.
One thing I would fix before merge, plus a few smaller ones.
A build from a git worktree loses the version and the commit
Same commit, same clone, same toolchain. Only the checkout kind differs:
| built in | main (before) | this PR |
|---|---|---|
| normal clone | brig v0.2.0-6-gef4aa8b |
brig v0.2.1-0.20260916082117-13e9bb7b33c8 (13e9bb7, ...) |
| linked worktree | brig v0.2.0-6-gef4aa8b |
brig dev (go1.26.5, darwin/arm64) |
The reason is in the toolchain. vcsGit declares its root as {filename: ".git", isDir: true}, and a linked worktree's .git is a file. isVCSRoot never matches, no repo root is found, and nothing is stamped. It is silent even under -buildvcs=true, since there is no repo to complain about.
So in a worktree we land exactly the state this PR sets out to remove: a binary that cannot name its commit. The old git describe line handled worktrees fine.
The release path is not affected. actions/checkout gives a normal clone, and I confirmed a tagged build prints its tag. This is about the dev workflow.
A line in docs/releasing.md would cover it. Another approach would be a Makefile fallback that stamps only when buildinfo comes back empty, which keeps the release binaries pure and gives worktree builds their commit back. What do you think?
The release checklist will not catch a +dirty build
The comment in .goreleaser.yaml names the risk correctly: the go mod tidy hook has to leave the tree clean. goreleaser validates dirtiness before it runs that hook though, so a tidy that touches go.sum produces +dirty binaries with nothing failing.
The check in docs/releasing.md says to look for a pseudo-version. v0.3.0+dirty is not a pseudo-version, so it passes. Adding "and no +dirty" to that line would close it, and a git diff --exit-code after the tidy hook would too.
.gitignore already covers /brig, /brigd and /dist/, so repeat make build and goreleaser's own output stay clean.
Nits
modifiedis always present inbrig version --jsonandomitemptyin brigd's response, so it is absent there when false.docs/brigd.mddocuments this, and the PR body calls them the same fields. Worth making the two agree, or saying why they differ.cmd/brig/main.gohas one 85-column line in the usage block (after the verb (brig ls --json)...). The rest of that block is at 80.theirs.Commit != mine.Commitflags a mismatch when one binary came fromgo install(no VCS data) and the other from a release archive, even at the same version, and the restart it suggests will not help. Comparing commits only when both are non-empty would cover it. Edge case, since the two normally ship together. Up to you.
On the commits
When you address these, could you fold each fix into the commit it belongs to and force-push, rather than adding fix commits on top? The 7 commits read well in order and it would be nice to keep them that way through merge.
…info brig and brigd learn their version from a variable stamped at link time: the tag under goreleaser, `git describe` under make, and "dev" for a plain `go build` or `go install`. None of those names the commit, so two binaries from the same tag print the same line, and an unstamped build prints nothing useful at all. The toolchain already embeds what is missing. Go 1.24 and later derive the main module version from the nearest reachable v* tag -- the tag on a tagged commit, a pseudo-version after one, +dirty on a modified tree -- and record the revision, its time and the platform beside it. This package reads that once and renders the one-line form both binaries will print, so neither needs a stamp. A linked git worktree is the exception: its .git is a file, which the toolchain does not recognise as a repository, so a build there embeds no VCS data. For that case the package takes git's own answers through -X variables, uses them only when the toolchain embedded nothing, and derives the version from them the way the toolchain would. Refs: #230 Signed-off-by: Alexandros Sapranidis <alexandros@nofire.ai>
`brig version` printed the stamped main.version and nothing else, so a release, a rebuilt release, and a local build with changes on top all looked alike, and a plain `go build` printed "brig dev". Bug reports asked for the commit by hand as a result. Read the build from the binary instead. The line keeps `brig <version>` as its first two words, so a reader taking the second field still gets a version, and adds the short commit, its date, the Go version and the platform in parentheses. Refs: #230 Signed-off-by: Alexandros Sapranidis <alexandros@nofire.ai>
A script that wants the commit a binary was built from had to parse the parentheses of the one-line form. Print the same build under the envelope the other read verbs use, kind Version, with the commit in full. commit and commitTime are absent rather than empty when the build carried no git history, so a consumer tests for the key. The flag is accepted in both positions, as it is for doctor, and version joins the verbs the global --json gate lets through. Any word other than --json is still refused. Refs: #230 Signed-off-by: Alexandros Sapranidis <alexandros@nofire.ai>
brigd answered the version op with the same stamped string brig printed, so a daemon left running across an upgrade could not be told from the binary talking to it once the two shared a tag. Read the build from the binary, as brig now does, and return the commit, its time and the modified flag beside the version. All three are added fields within protocol version 1, which is what the protocol permits. The version value changes spelling from 0.2.0 to v0.2.0; nothing in this repository reads it yet. modified is sent on every version answer, true or false, as `brig version --json` prints it, and on no other op's answer. Refs: #230 Signed-off-by: Alexandros Sapranidis <alexandros@nofire.ai>
The bug template asks for `brig doctor` output, and the report did not say which brig produced it. A daemon left running across an upgrade was invisible too: doctor saw a socket and a lock holder, and nothing about the code behind them. Open the report with the build, the same line `brig version` prints, and ask a running brigd which build it is over its version op. A daemon from a build other than this brig's is marked !! with a restart as its fix. A daemon that does not answer is noted and left ok: the socket check is about the socket, and a failed probe is not evidence of stale code. This is the first request brig sends to brigd; the op is read-only. The commit and the modified flag are compared only when both builds name a commit. A binary from `go install` names none, and restarting brigd would not give it one. Refs: #230 Signed-off-by: Alexandros Sapranidis <alexandros@nofire.ai>
Neither binary reads the variable any more: both take their version, commit and modified flag from what the Go toolchain embeds at build time. The linker ignored the orphaned -X, so the stamp was already inert; drop it from make and goreleaser so nobody wonders which of the two the binary reports. make passes git's commit, commit time, nearest tag and modified flag to internal/buildinfo in their place. The binary reads them only when the toolchain embedded nothing, which is a build in a linked git worktree. goreleaser passes none, so a release binary reports only what the toolchain embedded. The goreleaser comment records the two conditions the embedded version now depends on: a full clone, so the tag is reachable, and a `go mod tidy` hook that leaves the tree clean, or every release reads as +dirty. Refs: #230 Signed-off-by: Alexandros Sapranidis <alexandros@nofire.ai>
The file existed so a reader had a version to look at, and the release workflow asserted it matched the tag so it could not drift. Now that the binary reports the tag the toolchain derived at build time, the file is a second copy of the same fact with nothing reading it. Delete it and its check. The releasing doc describes what replaces the bump: a tag for a release, a prerelease tag for a candidate, a tag on a maintenance branch for a patch, and what a build between tags prints. The workflow runs `go mod tidy` before goreleaser and fails on a diff. goreleaser checks the tree before its hooks run, so a tidy hook that changed go.sum would otherwise build every binary as +dirty with nothing failing. Refs: #230 Signed-off-by: Alexandros Sapranidis <alexandros@nofire.ai>
13e9bb7 to
38c3713
Compare
|
@pmoust good question. It does, and more than we were using -- with no Our The catch is that the stamp only exists in goreleaser-built binaries. I would keep both: read |
`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>
3 participants
Summary
brig versionprinted one token, stamped intomain.versionat link time:the tag under goreleaser,
git describeunder make, anddevfor a plaingo buildorgo install. None of those named the commit, so two binariesfrom one tag printed the same line, a modified tree looked like a clean one,
and bug reports asked for the commit by hand.
Read the build from the binary instead. Go 1.24 and later derive the main
module version from the nearest reachable
v*tag and embed the commit, itstime and the modified flag beside it. Both binaries now report that, the
stamp is gone, and with it the VERSION file: the tag is the version.
Output changes once, from
brig 0.2.0tobrig v0.2.0.docs/cli.mdalready documented the
vform. The first two words staybrig <version>,so a reader taking the second field still gets a version.
Each commit stands alone: it builds, vets and passes its tests without the
ones after it. Reviewing in order is the intended path.
Related issues
Closes #230
Changes
internal/buildinfo: reads version, commit, commit time, modified flag, Goversion and platform from
debug.ReadBuildInfo, withdevand no commitas the fallback for a build without git history.
brig versionprints that build on one line.--jsonprints it under thestandard envelope, kind
Version, in either flag position.versionop returnscommit,commitTimeandmodifiedbesideversion; added fields within protocol version 1.brig doctoropens with abrigrow and asks a running brigd its build. Adaemon from a different build is
!!with a restart as the fix. This isthe first request brig sends to brigd, and the op is read-only.
.goreleaser.yamldrop-X main.version. The goreleasercomment records the two conditions the embedded version depends on: a full
clone, and a
go mod tidyhook that leaves the tree clean.VERSIONand its tag-matches check in the release workflow are deleted.docs/releasing.mddescribes the tag-only flow: release, releasecandidate, maintenance branch, and what a build between tags prints.
Checklist
make allpasses (vet, test, build)script/smoke.shpasses (340 ok, 0 failed)go test ./... -race, if the change touches concurrency, subprocesses or the daemonI have exercised the change against a real runtime (-- nothing in the run, exec or credential path changedbrig run <agent>), if it touches the run, exec or credential pathThe change was written with an AI assistant; the author reviewed every line.
🤖 Generated with Claude Code