Conversation
|
@claude-fleet review |
|
Review task queued. Task: cd0209f1-64db-4ba8-b46b-ba8c3305ea04 |
There was a problem hiding this comment.
Comment review. CI/docs only, no Go touched. Reviewed at b0f8d10 against merge-base ef4aa8b: read channel.yml, .goreleaser.yaml, render-cask.py, and both docs; ran the renderer against dummy archives to confirm output shape.
What I verified holds
- Archive name coupling lines up. goreleaser's archive
name_template(brig-{{.Version}}-{{.Os}}-{{.Arch}}) and the--archive brig-$V-...names passed torender-cask.pyboth resolve to the sameCHANNEL_VERSIONstring, so the cask'surl/sha256point at the assets that actually get uploaded. In--snapshot,.Versionis the renderedsnapshot.version_template, and that template returnsCHANNEL_VERSIONverbatim viaenvOrDefault. A local--snapshotwith the env unset keeps{{ incpatch .Version }}-snapshot, so the existing snapshot path is unchanged. - Moving-tag / no-release-trigger claim.
channel-mainandchannel-experimentaldon't matchv*, sorelease.ymlisn't started andskip_upload: autoon the stable cask is untouched. - Upgrade-on-any-difference premise. Homebrew treats a cask as outdated on version inequality, not greater-than, so two same-day builds differing only in short SHA still upgrade. The date/SHA scheme is sufficient.
- Renderer output.
conflicts_withlists stable + the other channel (not itself);depends_onemits hull@channel + cosign; completions reordered bash/fish/zsh;on_macos/on_linuxin order. Renders cleanly. fetch-depth: 0is needed for goreleaser tag history and forgit rev-parse --short HEADin the version step.VERSIONis0.2.0, read correctly.
Gates
No Go changed; no unit surface. I did not run actionlint, goreleaser, or brew style (not installed here) — see not_checked. Author reports all three clean.
Findings
One should-fix robustness issue on concurrent cross-channel publishes; details inline. Smaller: the goreleaser before hook re-runs go mod tidy + go test ./... on every merge to main (already covered by ci.yml), adding minutes per merge — intentional-looking, but worth a note.
Happy to approve once you've weighed the tap-push race; the rest is solid.
Not checked:
- brew style / brew audit on the rendered cask, and Homebrew's acceptance of
@cask tokens and nested on_macos/on_linux + on_arm/on_intel stanzas on target platforms (no brew available; author reports brew style clean) - actual
goreleaser release --snapshotrun with CHANNEL_VERSION set, and that emitted archive filenames equal CHANNEL_VERSION (author reports verified; confirmed only by template inspection) - actionlint on the new workflow (author reports clean)
- first live run: HOMEBREW_TAP_GITHUB_TOKEN presence/permissions and the tap push against the real homebrew-brig repo
- hull-side casks (hull@main / hull@experimental) that the depends_on references, from brig-sh/hull#75
Task cd0209f1-64db-4ba8-b46b-ba8c3305ea04 on rp1, model executor default, effort high.
| git add "Casks/brig@$CHANNEL.rb" | ||
| git diff --cached --quiet && { echo "the cask is unchanged"; exit 0; } | ||
| git commit -m "chore(cask): brig@$CHANNEL $V" | ||
| git push |
There was a problem hiding this comment.
should_fix: Concurrent main + experimental publishes can race on the tap push. The concurrency group is scoped per channel (channel-main vs channel-experimental, line 36), so a push-to-main run and a workflow_dispatch (experimental) run execute at the same time. Both git clone --depth 1 the tap's default branch (line 159) and git push here with no fetch/rebase/retry. If the two tap-update steps overlap, the first push wins and the second is rejected non-fast-forward; that job fails and leaves its channel's cask stale, with an error that points at git internals rather than the cause. The stable cask sidesteps this by opening PRs. Fix: put the tap write under a single shared concurrency group, or git pull --rebase origin <branch> and retry before pushing.
b0f8d10 to
ff802cd
Compare
|
This is great improvement, can I ask to hold a bit for this PR until we sort out #230 which I feel will simplify this PR as well by removing the need to encode the version and that when we have the snapshot it would report the proper version including the build commit. |
ff802cd to
8a98351
Compare
|
Fixed. The tap push now rebases and retries rather than pushing once: A shared concurrency group would not have covered it: brig and hull each publish two channels into the one tap, and a concurrency group is per workflow -- these are four workflows in two repositories, so there is no group they can all join. Rebase is the thing that works across all four, and they touch different files so it always applies. brig-sh/hull#75 got the same fix. The same push also carries the renderer fixes from the hull review: the DO NOT EDIT banner now names the path the script was invoked by, so it resolves in both repositories, and a cask rendered with no |
|
Held, and that is a fair ask -- #230 is the better-shaped fix for half of this, so waiting costs nothing. I have put this and brig-sh/hull#75 back to draft so nobody merges them past you. One thing worth separating before you design #230, because I think it removes less of this PR than it looks like it does. The version string here does two jobs, and they come apart: Diagnostics -- what commit is this binary. #230 removes this entirely, and better than encoding it in a token: Homebrew upgrade detection -- is there something newer. This one #230 does not reach. So the string can get shorter but not go away. Once #230 lands I would expect this to drop to something like If #230 ends up making Unrelated, and already pushed: the tap-push race claude-fleet raised is fixed (rebase and retry, since a concurrency group cannot span two repositories), as are the renderer nits from the hull review. So this is waiting only on #230, not on review comments. #228 and brig-sh/hull#74 are the port publishing itself and do not depend on any of this -- they are green and not held. |
asapranidis
left a comment
There was a problem hiding this comment.
#230 is fixed on main by #231. VERSION is removed. brig version reports the version the Go toolchain embeds: the tag on a tagged commit, otherwise a pseudo-version (v0.2.1-0.<commit time>-<commit>), with +dirty on a modified tree, plus the commit.
Steps to update this PR
- Rebase onto main; do not merge main in. Expected conflicts:
.goreleaser.yaml: main removed-X main.version={{.Version}}from both builds. Take main'sldflags; keep thesnapshot.version_templatechange.docs/releasing.md: main rewrote the start of the file. Keep the new "The prerelease channels" section.
- Replace "Compose the version" with the version the binary reports, after
actions/setup-go(inline, L72). - Check that
go mod tidyleaves the tree clean, and skip goreleaser'sbeforehooks (inline, L89). - Upload the new assets before deleting the old ones (inline, L109).
- Checkout credentials and experimental refs (inline, L60),
inputs.refinjection (inline, L49), cask conflicts (inline,docs/install.mdL56). - Update text that describes the version as last release + channel + date + short SHA: the comment at
channel.ymlL67-71, anddocs/releasing.mdif it describes the format.
Version string
Requirements from the earlier thread: a new string for every build, so brew upgrade detects it, and unique asset names. The pseudo-version meets both. It contains the commit and orders by commit time. It is also what brig version prints, so brew info brig@main and brig version show the same value.
brig@mainandbrig@experimentalbuilt from the same commit get the same version. They are different casks, with assets under different tags, so nothing collides.- Re-promoting the same commit does not trigger an upgrade. The code is the same.
- Promoting a tagged commit gives the tag, e.g.
0.3.0.
hull#75
Steps 3-5 apply there as well. Its version step reads hull's own VERSION, which #231 does not touch.
| - name: Compose the version | ||
| id: version | ||
| run: | | ||
| set -euo pipefail | ||
| BASE="$(cat VERSION)" | ||
| SHORT="$(git rev-parse --short HEAD)" | ||
| echo "value=${BASE}-${{ steps.channel.outputs.name }}.$(date -u +%Y%m%d).${SHORT}" >> "$GITHUB_OUTPUT" |
There was a problem hiding this comment.
VERSION was removed in #231, so cat VERSION fails. Read the version from the binary instead. This needs Go, so move the step after actions/setup-go and after the tidy check (L89):
- name: Read the version
id: version
run: |
set -euo pipefail
go build -o "$RUNNER_TEMP/brig" ./cmd/brig
v="$("$RUNNER_TEMP/brig" version --json | jq -r '.data.version | ltrimstr("v")')"
echo "value=$v" >> "$GITHUB_OUTPUT"On main this gives 0.2.1-0.20260916131706-003e6d17deb7. goreleaser names the archives from CHANNEL_VERSION, so the asset names, the cask version and brig version all show this one string.
The comment at L67-71 needs updating: the string no longer has the channel name or the build date.
| - uses: goreleaser/goreleaser-action@v7 | ||
| with: | ||
| version: latest | ||
| args: release --snapshot --clean --skip=sbom,sign | ||
| env: | ||
| CHANNEL_VERSION: ${{ steps.version.outputs.value }} |
There was a problem hiding this comment.
goreleaser checks the git tree before it runs the before hooks. If the go mod tidy hook changes go.mod or go.sum, the binaries embed +dirty and stop matching CHANNEL_VERSION. A snapshot build does not fail on a dirty tree, so nothing catches it.
Add the check release.yml now runs, before the version step:
- name: Check the tree stays clean after go mod tidy
run: |
go mod tidy
git diff --exit-codeThen skip the hooks here. go test ./... already runs in CI on every merge to main:
args: release --snapshot --clean --skip=before,sbom,sign| if gh release view "$TAG" >/dev/null 2>&1; then | ||
| # The old assets carry the previous build's version in their name, | ||
| # so --clobber does not replace them and they would accumulate | ||
| # until the release listed every build ever made. | ||
| gh release view "$TAG" --json assets -q '.assets[].name' | while read -r asset; do | ||
| [ -n "$asset" ] && gh release delete-asset "$TAG" "$asset" --yes | ||
| done | ||
| gh release edit "$TAG" --prerelease --target "$(git rev-parse HEAD)" \ | ||
| --title "$TITLE" --notes "$NOTES" | ||
| else | ||
| gh release create "$TAG" --prerelease --target "$(git rev-parse HEAD)" \ | ||
| --title "$TITLE" --notes "$NOTES" | ||
| fi | ||
| # checksums.txt as well as the archives: the tap's CI asks each | ||
| # release whether it really published the sha256 its cask claims, | ||
| # and a channel cask has to answer that the same way a stable one | ||
| # does. | ||
| gh release upload "$TAG" dist/*.tar.gz dist/checksums.txt --clobber |
There was a problem hiding this comment.
The old assets are deleted before the new ones are uploaded, and the tap names the old files until "Update the tap" finishes. During that window:
brew install --cask brig@maingets a 404.- A tap CI run started by a hull channel push fails the checksum job for
brig@main.
Order:
- Upload the new assets.
- Update the tap.
- In a step after "Update the tap", delete the assets whose names are not in
dist/.
checksums.txt keeps its name and is replaced in place, so a short window remains until the tap push lands.
| - uses: actions/checkout@v7 | ||
| with: | ||
| ref: ${{ steps.channel.outputs.ref }} | ||
| # goreleaser reads the tag history to derive the version this build | ||
| # is the successor to, so a shallow clone gives it the wrong answer. | ||
| fetch-depth: 0 |
There was a problem hiding this comment.
fetch-depth: 0is still needed: the Go toolchain derives the pseudo-version from the nearest tag. Update the comment to give that reason.- On
workflow_dispatchthis checks out the promoted ref.script/render-cask.pyand.goreleaser.yaml, including any hooks it defines, then come from that ref rather than main:- A ref cut before this PR has neither the renderer nor the
CHANNEL_VERSIONtemplate, so it cannot be promoted. - Code from a contributor's ref runs in a job with
contents: write, and by default checkout leaves that token in the git config.
- A ref cut before this PR has neither the renderer nor the
Suggested:
persist-credentials: false. The tag push at L105-106 then needs the token passed explicitly.- Check out the workflow's own commit (
github.workflow_sha) to a separate path. Runrender-cask.pyfrom there and pass goreleaser--configfrom there. - Move "Update the tap" to a second job that receives only the rendered cask as an artifact.
| echo "ref=${{ github.event.inputs.ref }}" | ||
| echo "source=${{ github.event.inputs.ref }}" |
There was a problem hiding this comment.
${{ github.event.inputs.ref }} is substituted into the script before bash runs, so a ref containing " or $(...) runs as shell. Pass it through env: and validate it:
[[ "$REF" =~ ^[A-Za-z0-9._/-]+$ ]] || { echo "not a ref: $REF" >&2; exit 1; }The same value reaches the cask via --source (L144) inside a Ruby string, where a " breaks the file.
| were asked to install one. Only one of the three casks can be installed at a | ||
| time; `brew uninstall --cask brig@main` and install the one you want. |
There was a problem hiding this comment.
Switching between casks fails in two cases:
-
Homebrew checks only the
conflicts_withof the cask being installed (Cask::Installer#check_conflicts).brig@maindeclares the conflict; the stablebrigcask does not.brew install --cask brigoverbrig@mainis therefore not blocked, and it fails later when Homebrew links the binaries. Add tohomebrew_casksin.goreleaser.yaml:conflicts: - cask: brig@main - cask: brig@experimental
-
brew uninstall --cask brig@mainleaveshull@maininstalled. The stablebrigthen pullshull, which conflicts withhull@main. The command needs both casks:brew uninstall --cask brig@main hull@main.
There was no way to install a build that is not a release. Trying a feature before it reached a tag meant building from source, which is a different ask from `brew install`, and it is the difference between somebody testing a branch and somebody saying they will. Two casks, and they are deliberately separate rather than one channel with two sources. `brig@main` follows the tip of main and is rebuilt on every merge, so `brew upgrade` follows what is coming. `brig@experimental` moves only when a maintainer promotes a ref to it, which is how an unmerged branch reaches a tester. Folding them together would mean a merge to main silently replacing the branch build a tester was halfway through checking. Each publishes to a moving tag whose assets are replaced in place, so a year of merges leaves one release rather than one per commit. Neither tag matches `v*`, so neither starts the release workflow, and `skip_upload: auto` on the stable cask is untouched. The cask is rendered by script/render-cask.py rather than by goreleaser. goreleaser's cask pipe renders for the version it is releasing, and a channel has no version of its own. The renderer takes the archive list explicitly, because brig ships four platforms and hull ships one, and it emits what `brew style` wants -- verified against real archives, with `brew style` reporting no offences and the rendered sha256 matching the built tarball. The channel cask is pushed to the tap rather than opened as a pull request: it is regenerated on every merge, and a PR per merge is noise nobody reads. The stable cask keeps its review. `snapshot.version_template` now honours CHANNEL_VERSION when it is set, so a channel build carries a version no other build shares -- Homebrew decides there is something to upgrade to from that string alone. A local `--snapshot` sets nothing and keeps the old spelling. Signed-off-by: Anastassios Nanos <ananos@nofire.ai>
8a98351 to
a79e24c
Compare
|
All six fixed, and #230 landing changed the shape of the first one for the better. Rebased onto current main. 1. I did check the claim rather than take it: a plain 2. 3. Assets deleted before the new ones are up. Reordered as you laid it out: upload, update the tap, then a final step removes the assets no longer in 4. The promoted ref, and the token. All three suggestions:
I did not split the tap update into a second job. With the config and the renderer pinned to the workflow commit and no credentials persisted, the remaining exposure is goreleaser's 5. Script injection on 6.
Checked: |
|
@claude-fleet review |
|
Cannot enqueue the review task (HTTP 400: bad request: spec contains an unexpanded shell substitution "$("$RUNNER_TEMP/brig" version --json | jq -r '.data.version | ltrimstr("v")" on line 490. A shell substitution placed in a JSON parameter never expands -- JSON does no shell processing, so this reaches the executor as literal text and ). No task enqueued. |
Summary
There was no way to install a build that is not a release. Trying a feature
before it reached a tag meant building from source, which is a different ask
from
brew install, and it is the difference between somebody testing a branchand somebody saying they will.
Two casks, kept separate rather than one channel with two sources.
brig@mainfollows the tip of main and is rebuilt on every merge, so
brew upgradefollows what is coming.
brig@experimentalmoves only when one of us promotesa ref to it, which is how an unmerged branch reaches a tester. Folding them
into one would mean a merge to main silently replacing the branch build a
tester was halfway through checking.
Each pulls the matching hull, from brig-sh/hull#75, because a feature usually
spans both. The port publishing in #228 is exactly that case: it needs a hull
change, so no brig-only channel could have carried it.
Related issues
Pairs with brig-sh/hull#75.
Changes
.github/workflows/channel.yml: builds and publishes a channel, on push tomainand onworkflow_dispatchwith a ref.channel-main,channel-experimental-- whose assets are replaced in place, so a year ofmerges leaves one release rather than one per commit. Neither matches
v*,so neither starts the release workflow and
skip_upload: autoon the stablecask is untouched.
script/render-cask.pyrenders the cask. goreleaser's cask pipe renders forthe version it is releasing, and a channel has no version of its own. It
takes the archive list explicitly, because brig ships four platforms and hull
ships one, and it emits what
brew stylewants, so no--fixpass.snapshot.version_templatehonoursCHANNEL_VERSIONwhen set. Homebrewdecides there is something to upgrade to from the version string alone, so
two builds of a moving channel must not share one. A local
--snapshotisunchanged.
docs/install.mdanddocs/releasing.md.Checklist
make allpasses (vet, test, build)script/smoke.shpassesI have added or updated tests covering the change-- a releaseworkflow has no unit test here; what could be run locally is below
I have run-- no Go code changedgo test ./... -raceI have exercised the change against a real runtime-- nothing in therun, exec or credential path changed
What was checked locally, since the workflow itself cannot be.
goreleaser release --snapshotwithCHANNEL_VERSIONset built all four archives underthe channel version, and without it still produced the old
0.2.1-snapshotspelling, so a local snapshot is unaffected. The renderer was run against those
real archives:
brew stylereported no offences, and thesha256in the caskmatches
shasum -a 256of the built tarball.actionlintis clean on the newworkflow.
What I could not check
The workflow has never executed: it needs the tap token and a merge to
mainto fire. The step most worth watching on the first run is the tap push, which
uses
HOMEBREW_TAP_GITHUB_TOKENdirectly rather than minting an App token theway the release does. That was deliberate -- a channel that cannot reach the
tap should fail loudly, and there is no release beside it to quietly go green
-- but it does mean the secret has to exist for the channel to work at all.
Two things worth your opinion
Naming. These are
brig@mainandbrig@experimental. You offered"current", "main" or "rolling" for the first; I took
@mainbecause it namesthe thing it follows and matches the branch, so there is nothing to look up.
Easy to change now, annoying once someone has it installed.
The tap gets a push, not a pull request. The stable cask opens a PR and a
maintainer merges it, which is right: it is what people install. A channel cask
is regenerated on every merge to main, and a PR per merge is noise nobody would
read, so this pushes. If you would rather have the review even here, the change
is small, but expect a PR per merge.