From a04c53fe26f660ce0024ef52bd0b62bb5e6aea86 Mon Sep 17 00:00:00 2001 From: Elie Habib Date: Fri, 24 Apr 2026 19:01:47 +0400 Subject: [PATCH] fix(build): pin sebuf plugin via PATH in `make generate` (#3371) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(build): pin sebuf plugin via PATH in `make generate` Without this, developers who have an older sebuf protoc-gen-openapiv3 binary installed via a package manager (Homebrew ships v0.7.0 at /opt/homebrew/bin) hit this failure on a fresh `make generate`: Failure: file ".../AviationService.openapi.yaml" was generated multiple times: once by plugin "protoc-gen-openapiv3" and again by plugin "protoc-gen-openapiv3" Root cause: `buf.gen.yaml` declares three `protoc-gen-openapiv3` invocations — the default yaml, a `format=json` variant, and a `bundle_only=true, strategy: all` unified bundle. Sebuf v0.11.x honors both `format=json` (emits .json extension) and `bundle_only=true` (skips per-service emission), so the three invocations write distinct files. Sebuf v0.7.x does NOT honor either option — it silently emits the same per-service .yaml filenames from all three plugins and buf rejects the collision. `Makefile: install-plugins` installs v0.11.1 (SEBUF_VERSION) to $HOME/go/bin. But the `generate` target doesn't prepend that to PATH, so `which protoc-gen-openapiv3` resolves to the stale Homebrew binary for anyone with both installed. Verified by `go version -m`: /opt/homebrew/bin/protoc-gen-openapiv3 — mod sebuf v0.7.0 /Users/eliehabib/go/bin/protoc-gen-openapiv3 — mod sebuf v0.11.1 Fix: prepend $$HOME/go/bin to PATH in the `generate` recipe. Matches what .husky/pre-push:151-153 already does before invoking this target, so CI and local behavior converge. No sebuf upstream bug. * fix(build): follow GOBIN-then-GOPATH/bin for plugin PATH prefix Reviewer (Codex) on PR #3371: the previous patch hardcoded \$HOME/go/bin, which is only the default fallback when GOBIN is unset AND GOPATH defaults to \$HOME/go. On machines with a custom GOBIN or a non-default GOPATH, `go install` targets a different directory — so hardcoding \$HOME/go/bin can force a stale binary from there to win over the freshly-installed SEBUF_VERSION sitting at the actual install location. Fix: resolve the install dir the same way `go install` does: GOBIN first, then GOPATH/bin. Shell expression: `go env GOBIN` returns an empty string (exit 0) when unset, so `||` alone doesn't cascade. Using explicit `[ -n "$gobin" ]` instead. Also dropped the misleading comment that claimed the pre-push hook used the same rule — it still hardcodes \$HOME/go/bin. Called that out in a note, but left the hook alone because its PATH prepend is belt-and-suspenders (only matters for locating `buf` itself; the Makefile's own recipe-level prepend decides plugin resolution). Verified on a machine with empty GOBIN: resolved → /Users/eliehabib/go/bin And \`make generate\` succeeds without manual PATH overrides. * fix(build): use first GOPATH entry for plugin PATH prefix Reviewer (Codex) on commit 6db0b53c2: the GOBIN-empty fallback used `$(go env GOPATH)/bin`, which silently breaks on setups where GOPATH is a colon-separated list. Example: GOPATH=/p1:/p2 previous code → "/p1:/p2/bin" ^ two PATH entries; neither is the actual install target /p1/bin `go install` writes binaries only into the first GOPATH entry's bin, so the stale-plugin case this PR is trying to fix can still bite. Fix: extract the first entry via `cut -d: -f1`. Matches Go's own behavior in cmd/go/internal/modload/init.go:gobin(), which uses filepath.SplitList + [0]. Verified: - Single-entry GOPATH (this machine) → /Users/eliehabib/go/bin ✓ - Simulated GOPATH=/fake/path1:/fake/path2 → /fake/path1/bin ✓ - make generate succeeds in both cases. * fix(build): resolve buf via caller PATH; prepend plugin dir only (review P1/P3) Codex P1: the previous recipe prepended the Go install dir to PATH before invoking `buf generate`, which also changed which `buf` binary ran. On a machine with a stale buf in GOBIN/$HOME/go/bin, the recipe would silently downgrade buf itself and reintroduce version-skew failures — the exact class of bug this PR was trying to fix. Fix: two-stage resolution. 1. `BUF_BIN=$(command -v buf)` resolves buf using the CALLER's PATH (Homebrew, Go install, distro package — whichever the developer actually runs day-to-day). 2. Invoke the resolved buf via absolute path ("$BUF_BIN"), with a PATH whose first entry is the Go install dir. That affects ONLY plugin lookup inside `buf generate` (protoc-gen-ts-*, protoc-gen-openapiv3) — not buf itself, which was already resolved. Adds a loud failure when `buf` is not on PATH: buf not found on PATH — run: make install-buf Previously a missing buf would cascade into a confusing error deeper in the pipeline. Codex P3: added tests/makefile-generate-plugin-path.test.mjs — scrapes the generate recipe text and asserts: - `command -v buf` captures buf before the PATH override - Missing-buf case fails loudly - buf is invoked via "$BUF_BIN" (absolute path) - GOBIN + GOPATH/bin resolution is present - Install-dir prepend precedes $$PATH (order matters) - The subshell expression resolves on the current machine Codex P2 (Windows GOPATH semicolon delimiter) is acknowledged but not fixed here — this repo does not support Windows dev per CLAUDE.md, the pre-push hook and CI are Unix-only, and a cross-platform implementation would require a separate Make detection or a platform-selected helper script. Documented inline as a known Unix assumption. Verified: - `make generate` clean - `command -v buf` → /opt/homebrew/bin/buf - protoc-gen-openapiv3 via plugin-PATH → ~/go/bin/protoc-gen-openapiv3 - New test suite 6/6 pass - npm run typecheck clean * fix(build): silence recipe comments with @# (review P2) Codex P2: recipe lines starting with `#` are passed to the shell (which ignores them) but Make still echoes them to stdout before execution. Running `make generate` printed all 34 comment lines verbatim. Noise for developers, and dilutes the signal when the actual error output matters. Fix: prefix every in-recipe `#` comment with `@` so Make suppresses the echo. No semantic change — all comments still read identically in the source. Verified: `make generate` now prints only "Clean complete!", the buf invocation line (silenced with @... would hide the invocation which helps debugging, so leaving that audible), and "Code generation complete!". * fix(build): fail closed when go missing; verify plugin is present (review High) Codex High: previous recipe computed PLUGIN_DIR from `go env GOBIN` / `go env GOPATH` without checking that `go` itself was on PATH. When go is missing: - `go env GOBIN` fails silently, gobin="" - `go env GOPATH` fails silently, cut returns "" - printf '%s/bin' "" yields "/bin" - PATH becomes "/bin:$PATH" — doesn't override anything - `buf generate` falls back to whatever stale sebuf plugin is on PATH, reintroducing the exact duplicate-output failure this PR was supposed to fix. Fix (chained in a single shell line so any guard failure aborts): 1. `command -v go` — fail with clear "install Go" message. 2. `command -v buf` — fail with clear "run: make install-buf". 3. Resolve PLUGIN_DIR via GOBIN / GOPATH[0]/bin. 4. `[ -n "$$PLUGIN_DIR" ]` — fail if resolution returned empty (shouldn't happen after the go-guard, but belt-and-suspenders against future shell weirdness). 5. `[ -x "$$PLUGIN_DIR/protoc-gen-ts-client" ]` — fail if the plugin isn't installed, telling the user to run `make install-plugins`. Catches the case where `go` exists but the user has never installed sebuf locally. 6. `PATH="$$PLUGIN_DIR:$$PATH" "$$BUF_BIN" generate`. Verified failure modes: - go missing → "go not found on PATH — run: ... install-plugins" - buf missing → "buf not found on PATH — run: make install-buf" - happy path → clean `Code generation complete!` Extended tests/makefile-generate-plugin-path.test.mjs with: - `fails loudly when go is not on PATH` - `verifies the sebuf plugin binary is actually present before invoking buf` - Rewrote `PATH override order` to target the new PLUGIN_DIR form. All 8 tests pass. Typecheck clean. * fix(makefile): guard all sebuf plugin binaries, not just ts-client proto/buf.gen.yaml invokes THREE sebuf binaries: - protoc-gen-ts-client - protoc-gen-ts-server - protoc-gen-openapiv3 (× 3 plugin entries) The previous guard only verified protoc-gen-ts-client was present in the pinned Go install dir. If the other two were missing from that dir (or only partially installed by a prior failed `make install-plugins`), `PATH="$PLUGIN_DIR:$PATH" buf generate` would fall through to whatever stale copy happened to be earlier on the caller's normal PATH — exactly the mixed-sebuf-version failure this PR is meant to eliminate. Fix: iterate every plugin name in a shell `for` loop. Any missing binary aborts with the same `Run: make install-plugins` remediation the previous guard showed. Tests: - Update the existing plugin-presence assertion to require all three binaries by name AND the `[ -x "$PLUGIN_DIR/$p" ]` loop pattern. - Add a cross-reference test that parses proto/buf.gen.yaml, extracts every `local:` plugin, and fails if any declared plugin is missing from the Makefile guard list. This catches future drift without relying on a human to remember the dependency. Closes the PR #3371 High finding that a `ts-server` or `openapiv3` missing from $PLUGIN_DIR would silently re-enable the stale-plugin bug. * fix(pre-push): don't shadow caller's buf with stale ~/go/bin/buf The proto-freshness hook's unconditional `export PATH="$HOME/go/bin:$PATH"` defeated the Makefile-side caller-PATH-first invariant: on machines with both a preferred buf (Homebrew, /usr/local, etc.) AND an older `go install buf@` leftover at `~/go/bin/buf`, the prepend placed the stale copy first. `make generate` then resolved buf via `command -v buf` and picked up the shadowed stale binary — recreating the mixed-version failure this PR is meant to eliminate. Fix: 1. Only prepend `$HOME/go/bin` when buf is NOT already on the caller's PATH. Now buf's Homebrew/system copy always wins; `~/go/bin/buf` is a pure fallback. 2. Widen the plugin-presence check to accept either a PATH-resolvable `protoc-gen-ts-client` OR the default go-install location `$HOME/go/bin/protoc-gen-ts-client`. `make generate` now resolves plugins via its own PLUGIN_DIR (GOBIN, then first-entry GOPATH/bin), so requiring them on PATH is too strict. 3. Drop the redundant plugin-only PATH prepend — the Makefile's own plugin-path resolution handles it authoritatively. Tests: add a regression guard that reads the hook, verifies the prepend is gated on `! command -v buf`, and explicitly asserts the OLD buggy pattern is not present. Closes the PR #3371 High finding about the hook's unconditional prepend defeating the Makefile-side caller-PATH-first invariant. --- .husky/pre-push | 20 +- Makefile | 59 ++++- tests/makefile-generate-plugin-path.test.mjs | 222 +++++++++++++++++++ 3 files changed, 298 insertions(+), 3 deletions(-) create mode 100644 tests/makefile-generate-plugin-path.test.mjs diff --git a/.husky/pre-push b/.husky/pre-push index 1e94fd0f2..5b3443897 100755 --- a/.husky/pre-push +++ b/.husky/pre-push @@ -151,10 +151,26 @@ fi echo "Running proto freshness check..." if git diff --name-only origin/main -- proto/ src/generated/ docs/api/ Makefile | grep -q .; then - if command -v buf >/dev/null 2>&1 || [ -x "$HOME/go/bin/buf" ]; then + # Only prepend $HOME/go/bin when buf is NOT already resolvable on the + # caller's PATH. An unconditional prepend would shadow a developer's + # preferred `buf` (e.g. Homebrew) with a potentially stale + # ~/go/bin/buf left over from an older `go install buf@`, which + # is the exact mixed-version failure this PR is trying to eliminate. + # Plugin resolution is handled inside `make generate` via a pinned + # PLUGIN_DIR, so the hook no longer needs to prepend for plugin + # discovery — only for the `buf` binary itself, and only when there's + # no other candidate. + if ! command -v buf >/dev/null 2>&1 && [ -x "$HOME/go/bin/buf" ]; then export PATH="$HOME/go/bin:$PATH" fi - if command -v buf &>/dev/null && command -v protoc-gen-ts-client &>/dev/null; then + # Plugin-presence check: consider a plugin "available" if it's on + # PATH OR at the default `go install` location ($HOME/go/bin). + # `make generate` resolves plugins via its own PLUGIN_DIR (GOBIN, + # then first-entry GOPATH/bin), which matches the $HOME/go/bin + # default on almost every dev setup. Anything exotic (custom GOBIN + # not on PATH) still runs `make generate` cleanly because the + # Makefile's own plugin-executable guard fires there. + if command -v buf &>/dev/null && { command -v protoc-gen-ts-client &>/dev/null || [ -x "$HOME/go/bin/protoc-gen-ts-client" ]; }; then make generate if ! git diff --exit-code src/generated/ docs/api/; then echo "" diff --git a/Makefile b/Makefile index 382e581bd..89732445d 100644 --- a/Makefile +++ b/Makefile @@ -54,7 +54,64 @@ lint: ## Lint protobuf files generate: clean ## Generate code from proto definitions @mkdir -p $(GEN_CLIENT_DIR) $(GEN_SERVER_DIR) $(DOCS_API_DIR) - cd $(PROTO_DIR) && buf generate + @# Ensure the Makefile-declared sebuf protoc plugins ($(SEBUF_VERSION)) + @# installed by `install-plugins` win over any stale sebuf binary that + @# a package manager (Homebrew, etc.) may have placed earlier on PATH. + @# Without this, `buf generate` spawns an older sebuf v0.7.x plugin + @# that ignores `bundle_only=true` / `format=json` and fails with + @# duplicate-output errors. + @# + @# Two-stage resolution: + @# + @# 1. Resolve `buf` using the CALLER's PATH so we pick up whatever + @# `buf` version was installed via Homebrew / go install / etc. — + @# whichever the developer actually runs day-to-day. We do NOT + @# want the Go install dir to override the buf binary itself; an + @# earlier version of this Makefile did, which could silently + @# downgrade `buf` on machines with a stale GOBIN copy. + @# + @# 2. Invoke the resolved `buf` via absolute path, but give it a + @# PATH whose FIRST entry is the Go install dir. This affects + @# only plugin lookup inside `buf generate` (protoc-gen-ts-*, + @# protoc-gen-openapiv3) — not `buf` itself, which is already + @# resolved. Plugins find the Makefile-pinned version first. + @# + @# Go install dir resolution mirrors `go install`'s own logic: + @# GOBIN first, then the FIRST entry of GOPATH + "/bin". `go install` + @# writes binaries only into the first GOPATH entry's bin dir — GOPATH + @# can be a path-list (colon-separated on Unix, semicolon on Windows), + @# so naïvely appending "/bin" to the whole value produces a bogus + @# path. The `cut -d:` fallback works on Linux/macOS shells; Windows + @# (MSYS/cmd) is not a supported dev platform for this repo, so the + @# Unix assumption is acceptable here. + @# + @# .husky/pre-push still prepends $$HOME/go/bin for the outer shell + @# discovering `buf` — that's a broader prepend (it affects the shell's + @# command resolution before `make` runs) and is harmless here because + @# this recipe resolves `buf` via its own PATH before building the + @# plugin PATH. + @command -v go >/dev/null 2>&1 || { echo 'go not found on PATH — make generate needs `go` to resolve the sebuf plugin install dir. Install Go, then run: make install-plugins' >&2; exit 1; } + @command -v buf >/dev/null 2>&1 || { echo 'buf not found on PATH — run: make install-buf' >&2; exit 1; } + @# All plugin-resolution, plugin-presence, and invocation in a single + @# shell line chained with `&&` so any failed guard aborts the recipe + @# BEFORE `buf generate` runs. `$$(go env GOBIN)` without -n guard + @# would silently become "/bin" when unset, failing to override PATH + @# and letting a stale sebuf on the normal PATH win — the exact + @# failure mode this PR is trying to prevent (Codex high-severity on + @# 9c0058a). The plugin-executable check covers ALL three sebuf + @# binaries invoked by proto/buf.gen.yaml (protoc-gen-ts-client, + @# protoc-gen-ts-server, protoc-gen-openapiv3) — guarding only one + @# would let `buf generate` fall through to a stale copy of the + @# others on PATH, recreating the mixed-version failure mode. Keep + @# this list in sync with proto/buf.gen.yaml. + cd $(PROTO_DIR) && \ + PLUGIN_DIR=$$(gobin=$$(go env GOBIN); if [ -n "$$gobin" ]; then printf '%s' "$$gobin"; else printf '%s/bin' "$$(go env GOPATH | cut -d: -f1)"; fi) && \ + [ -n "$$PLUGIN_DIR" ] || { echo 'Could not resolve Go install dir from GOBIN/GOPATH — refusing to run buf generate without a pinned plugin location.' >&2; exit 1; } && \ + for p in protoc-gen-ts-client protoc-gen-ts-server protoc-gen-openapiv3; do \ + [ -x "$$PLUGIN_DIR/$$p" ] || { echo "$$p not found at $$PLUGIN_DIR/. Run: make install-plugins" >&2; exit 1; }; \ + done && \ + BUF_BIN=$$(command -v buf) && \ + PATH="$$PLUGIN_DIR:$$PATH" "$$BUF_BIN" generate @echo "Code generation complete!" breaking: ## Check for breaking changes against main diff --git a/tests/makefile-generate-plugin-path.test.mjs b/tests/makefile-generate-plugin-path.test.mjs new file mode 100644 index 000000000..b44a8450f --- /dev/null +++ b/tests/makefile-generate-plugin-path.test.mjs @@ -0,0 +1,222 @@ +// Regression guard for the `generate` target's plugin-path resolution. +// +// The Makefile's `generate` recipe must satisfy two invariants: +// +// 1. `buf` is resolved via the CALLER's PATH. Overriding buf's own +// location can silently downgrade the build tool on machines with a +// stale binary in GOBIN. +// 2. Proto plugins (protoc-gen-ts-*, protoc-gen-openapiv3) resolve +// from the Go install dir FIRST — GOBIN when set, otherwise the +// first entry of GOPATH + "/bin". This mirrors `go install`'s own +// resolution order. +// +// This suite scrapes the recipe text from the Makefile and asserts the +// shell expression matches both invariants. It does not shell out to +// `make generate` — that's covered by the pre-push proto-freshness hook. +// We're guarding against future Makefile edits that break the pattern +// without having to run the whole proto pipeline to notice. +// +// Closes the PR #3371 P3 finding about missing automated coverage for +// the path-resolution behavior. + +import { strict as assert } from 'node:assert'; +import { test, describe } from 'node:test'; +import { readFileSync } from 'node:fs'; +import { resolve, dirname } from 'node:path'; +import { fileURLToPath } from 'node:url'; +import { execSync } from 'node:child_process'; + +const __dirname = dirname(fileURLToPath(import.meta.url)); +const MAKEFILE = readFileSync(resolve(__dirname, '../Makefile'), 'utf-8'); + +function extractGenerateRecipe() { + // Match `generate:` through the next blank line or non-indented line. + const m = MAKEFILE.match(/^generate:.*?\n((?:\t[^\n]*\n|#[^\n]*\n|\s*\n)+)/m); + if (!m) throw new Error('generate target not found in Makefile'); + return m[0]; +} + +describe('Makefile generate target — plugin path resolution', () => { + const recipe = extractGenerateRecipe(); + + test('resolves buf via command -v before invoking it', () => { + // `command -v buf` must appear before the PATH override so the + // caller's buf is captured first. Any version pinned by PATH + // manipulation below only affects plugin resolution. + assert.match( + recipe, + /BUF_BIN=\$\$\(command -v buf\)/, + 'generate recipe must resolve buf via `command -v buf` before invoking it', + ); + }); + + test('fails loudly when buf is not on PATH', () => { + // Must not silently fall through when buf is absent. + assert.match( + recipe, + /buf not found on PATH/i, + 'generate recipe must emit a clear error when buf is missing', + ); + }); + + test('fails loudly when go is not on PATH', () => { + // `go env GOBIN` failing silently would let PLUGIN_DIR resolve to + // "/bin" (empty + "/bin" suffix), which doesn't override PATH — a + // stale sebuf on the normal PATH would win and the duplicate-output + // failure this PR is trying to prevent would come back. Codex + // high-severity on commit 9c0058a. + assert.match( + recipe, + /command -v go/, + 'generate recipe must check that `go` is on PATH before attempting plugin resolution', + ); + assert.match( + recipe, + /go not found on PATH/i, + 'generate recipe must emit a clear error when go is missing', + ); + }); + + test('verifies EVERY sebuf plugin binary referenced by buf.gen.yaml is present', () => { + // `go` can be installed without the user having ever run + // `make install-plugins`. Guarding only one plugin would let buf + // fall through to a stale copy of the OTHERS on the normal PATH + // and recreate the mixed-version bug this PR is meant to prevent. + // The list here must stay in sync with proto/buf.gen.yaml. + const required = ['protoc-gen-ts-client', 'protoc-gen-ts-server', 'protoc-gen-openapiv3']; + for (const bin of required) { + // The guard iterates a shell `for` loop, so the literal plugin + // name must appear in the recipe's plugin list AND the + // remediation string must reference install-plugins. + assert.ok( + recipe.includes(bin), + `generate recipe must include ${bin} in the plugin-executable guard list`, + ); + } + // The loop must check each entry with `[ -x "$PLUGIN_DIR/$p" ]` + // and bail with a message that points users to `make install-plugins`. + assert.match( + recipe, + /\[ -x "\$\$PLUGIN_DIR\/\$\$p" \]/, + 'generate recipe must verify each plugin is executable via [ -x "$PLUGIN_DIR/$p" ]', + ); + assert.match( + recipe, + /Run: make install-plugins/, + 'generate recipe must tell the user the remediation when a plugin is missing', + ); + }); + + test('plugin guard list covers every plugin declared in buf.gen.yaml', () => { + // Cross-reference proto/buf.gen.yaml plugin entries against the + // Makefile's guard list. If buf.gen.yaml ever adds a new `local:` + // plugin (e.g. a future protoc-gen-*), the guard must grow too — + // otherwise the new binary can silently fall through to a stale + // PATH copy. + const BUF_GEN = readFileSync(resolve(__dirname, '../proto/buf.gen.yaml'), 'utf-8'); + const declared = new Set(); + for (const m of BUF_GEN.matchAll(/^\s*-\s*local:\s*(\S+)\s*$/gm)) { + declared.add(m[1]); + } + assert.ok(declared.size > 0, 'buf.gen.yaml must declare at least one local plugin'); + for (const bin of declared) { + assert.ok( + recipe.includes(bin), + `buf.gen.yaml declares '${bin}' but the Makefile guard does not check it — stale ${bin} on PATH could still win`, + ); + } + }); + + test('invokes buf via absolute path (via "$BUF_BIN"), not via PATH lookup', () => { + // Using "$$BUF_BIN" generate ensures the plugin-PATH override + // (added only for this command) does not also redirect which `buf` + // binary runs. The whole point of the two-stage resolution. + assert.match( + recipe, + /"\$\$BUF_BIN" generate/, + 'generate recipe must invoke buf via absolute path "$BUF_BIN"', + ); + }); + + test('prepends GOBIN-or-GOPATH/bin to PATH for plugin lookup', () => { + // Plugin resolution follows `go install`'s own rule: + // GOBIN when set, otherwise GOPATH/bin using the FIRST entry of + // GOPATH (GOPATH can be a path-list). + assert.ok(recipe.includes('go env GOBIN'), + 'generate recipe must consult `go env GOBIN`'); + assert.ok(recipe.includes('go env GOPATH | cut -d:'), + 'generate recipe must extract first GOPATH entry via `go env GOPATH | cut -d:`'); + assert.ok(recipe.includes(':$$PATH"'), + 'generate recipe must prepend to $$PATH (install-dir:$$PATH, not the other way around)'); + }); + + test('PATH override order: install-dir comes first, then original PATH', () => { + // PLUGIN_DIR must appear BEFORE $$PATH in the PATH assignment. + // Reversing them would let any earlier PATH entry (e.g. Homebrew + // plugins) shadow the Makefile-pinned version. + const pathAssignMatch = recipe.match(/PATH="\$\$PLUGIN_DIR:\$\$PATH"/); + assert.ok( + pathAssignMatch, + 'recipe must contain PATH="$$PLUGIN_DIR:$$PATH" — resolved plugin dir first, original PATH second', + ); + // Cross-check: PLUGIN_DIR must have been computed before the PATH + // assignment uses it. + const pluginDirAssignIdx = recipe.indexOf('PLUGIN_DIR='); + const pathAssignIdx = recipe.indexOf('PATH="$$PLUGIN_DIR'); + assert.ok(pluginDirAssignIdx >= 0, 'recipe must set PLUGIN_DIR'); + assert.ok(pathAssignIdx > pluginDirAssignIdx, + 'PATH assignment must come AFTER the PLUGIN_DIR computation'); + // The GOBIN lookup happens in the PLUGIN_DIR assignment, which + // precedes the PATH assignment — verified above. + }); + + test('pre-push hook does not unconditionally prepend $HOME/go/bin', () => { + // The Makefile's caller-PATH-first invariant is only meaningful + // if the hook invoking it doesn't first shadow the caller's `buf`. + // An unconditional `export PATH="$HOME/go/bin:$PATH"` would let a + // stale `~/go/bin/buf` (from an old `go install`) win over a newer + // Homebrew-installed `buf`, defeating the whole point of this PR. + // + // The hook MUST guard the prepend on "buf is not already on PATH" + // so the prepend only fires as a fallback when buf has no other + // candidate. + const HOOK = readFileSync(resolve(__dirname, '../.husky/pre-push'), 'utf-8'); + // Locate the proto-freshness block by its echo line. + const start = HOOK.indexOf('Running proto freshness check'); + assert.ok(start >= 0, 'pre-push hook must contain the proto-freshness block'); + const block = HOOK.slice(start, start + 2000); + // The prepend MUST be gated on `! command -v buf` so it only fires + // when buf has no other candidate. Any `export PATH="$HOME/go/bin:$PATH"` + // inside this block must appear inside an `if ! command -v buf ...` + // arm — never directly under the block or under a bare `if command -v buf`. + assert.match( + block, + /if\s+!\s+command\s+-v\s+buf[^\n]*\n[^\n]*export PATH="\$HOME\/go\/bin:\$PATH"/, + 'pre-push hook must gate the $HOME/go/bin prepend on `! command -v buf` so it only fires as a fallback — ' + + 'otherwise a stale ~/go/bin/buf would shadow the caller\'s preferred buf binary', + ); + // Explicit regression guard for the PRIOR buggy pattern that this + // PR is replacing. The old hook did + // `if command -v buf ... || [ -x "$HOME/go/bin/buf" ]; then export PATH=...` + // which ALWAYS prepended whenever buf was reachable anywhere — + // exactly the stale-buf-wins failure mode. + assert.ok( + !/if\s+command\s+-v\s+buf[^\n]*\|\|\s*\[\s*-x\s+"\$HOME\/go\/bin\/buf"\s*\][^\n]*;\s*then\s*\n\s*export PATH="\$HOME\/go\/bin:\$PATH"/.test(block), + 'pre-push hook must not use the old `buf-on-PATH-OR-at-~/go/bin -> prepend` pattern — it shadowed Homebrew buf with stale go-install buf', + ); + }); + + test('path expansion succeeds on current machine', () => { + // The shell expression is syntactically correct and resolves to + // an existing directory on this runner. Catches obvious typos + // (e.g. mismatched parens, wrong subshell syntax) at test time + // instead of at first `make generate` attempt. + const out = execSync( + `bash -c 'gobin=$(go env GOBIN); if [ -n "$gobin" ]; then printf "%s" "$gobin"; else printf "%s/bin" "$(go env GOPATH | cut -d: -f1)"; fi'`, + { encoding: 'utf-8' }, + ).trim(); + assert.ok(out.length > 0, 'install-dir expression must produce a non-empty path'); + assert.ok(out.endsWith('/bin') || out.includes('go'), + `install-dir "${out}" should end with /bin or contain "go"`); + }); +});