fix(fmt): the formatter gates walked the working tree, not the repo (cli#549)

.cursor/BUGBOT.md

@@ -125,6 +125,15 @@ Two things make this repo unusual and should shape every finding:
  pinned standalone binaries — `errcheck`, `gofmt -s`, `goimports`, `ineffassign`, `misspell`,
  `staticcheck`, plus `deadcode-check.sh`, `file-budget.sh`, `check-style.sh`. Don't infer
  coverage from that file.
+- **The two formatters run via `make fmt-check`, not inline in the workflow** (cli#549), and
+ they scope to `git ls-files '*.go'` rather than `.`. Both are deliberate: `.` walked untracked
+ scratch directories, and one definition of the file set is what stops local and CI
+ disagreeing. `scripts/format.sh` fails closed (exit 2) outside a work tree or on an empty file
+ list — do not "simplify" either guard away. `run_formatter` returns a status and
+ writes to a temp file rather than being captured in `$( )`: a function that
+ `exit`s inside a command substitution ends only the subshell, and the first cut
+ of this script shipped exactly that false green. `make fmt-selftest`
+ (`scripts/tests/format-verify.sh`) is the guard; it fails on the old shape.
 - **`staticcheck` runs `-checks all,-ST1005` deliberately** — do not flag error-string
  capitalisation or punctuation. It is a tracked, intentional exclusion (cli#279).
 - `internal/submit/client.go:78` — `InsecureSkipVerify` is intentional for cluster-internal

.github/workflows/build.yml

@@ -77,10 +77,19 @@ jobs:
  shellcheck --shell=sh --severity=error scripts/install.sh
  shellcheck --shell=bash --severity=error scripts/check-style.sh
  shellcheck --shell=bash --severity=error scripts/check-tool-pins.sh
+ shellcheck --shell=bash --severity=error scripts/format.sh
+ shellcheck --shell=bash --severity=error scripts/tests/format-verify.sh
  dash -n scripts/install.sh
  bash -n scripts/tests/install-verify.sh
  shellcheck --shell=bash --severity=error scripts/tests/install-ps1-verify.sh
  bash -n scripts/tests/install-ps1-verify.sh
+ # format.sh's own fail-closed properties. Formatters are stubbed, so this is
+ # hermetic and needs no Go toolchain — which is why it lives in this job
+ # rather than Lint. It exists because the first cut of format.sh reported
+ # "clean" on a formatter that never ran (#550 review).
+ - name: Formatter-gate harness (fail-closed / tracked-files scope)
+ run: bash scripts/tests/format-verify.sh
+
  - name: Verification harness (mandatory cosign / fail-closed)
  run: bash scripts/tests/install-verify.sh
  # Same property on Windows (backend#2078). pwsh is preinstalled on the
@@ -152,29 +161,18 @@ jobs:
  go install github.com/kisielk/errcheck@v1.20.0
  errcheck ./...
 
- - name: gofmt -s
- run: |
- drift="$(gofmt -s -l .)"
- if [ -n "$drift" ]; then
- echo "::error::gofmt -s drift in:"
- echo "$drift" | sed 's/^/ /'
- echo "::error::run \`make fmt\` to fix"
- exit 1
- fi
-
- # goimports -local: enforce the stdlib / third-party / our-own import
- # grouping that .golangci.yml's local-prefixes already declares. gofmt
- # doesn't check grouping, so drift accumulated silently until now.
- - name: goimports -local
- run: |
- go install golang.org/x/tools/cmd/goimports@v0.48.0
- drift="$(goimports -local github.com/tracebloc/cli -l .)"
- if [ -n "$drift" ]; then
- echo "::error::goimports (import grouping) drift in:"
- echo "$drift" | sed 's/^/ /'
- echo "::error::run \`make fmt\` to fix"
- exit 1
- fi
+ # gofmt -s (simplification) + goimports -local (the stdlib / third-party /
+ # our-own import grouping that .golangci.yml's local-prefixes declares;
+ # gofmt does not check grouping).
+ #
+ # `make fmt-check`, not an inline copy: both formatters now scope to
+ # `git ls-files '*.go'` instead of `.` (cli#549), and a second inline copy
+ # of that scope here is how local and CI start disagreeing about which
+ # files are gated. It also drops the restated goimports pin — the version
+ # is declared once, by GOIMPORTS_VERSION in the Makefile, which is what
+ # check-tool-pins.sh now enforces for this tool too.
+ - name: gofmt -s + goimports -local
+ run: make fmt-check
 
  - name: ineffassign
  run: |

Makefile

@@ -33,7 +33,8 @@ help:
 	@echo " build build ./tracebloc"
 	@echo " install go install ./cmd/tracebloc"
 	@echo
-	@echo " individual: vet test lint lint-full fmt fmt-check schema-check"
+	@echo " individual: vet test lint lint-full fmt fmt-check fmt-selftest"
+	@echo " schema-check"
 	@echo " vulncheck deadcode file-budget check-style clean"
 	@echo " cover cover-integration cover-merge test-integration"
 
@@ -50,7 +51,7 @@ help:
 # * schema-check — fetches data-ingestors at the pinned ref.
 # * deadcode — another `go run tool@version` fetch.
 .PHONY: check
-check: vet test-fast fmt-check file-budget check-style check-tool-pins
+check: vet test-fast fmt-check fmt-selftest file-budget check-style check-tool-pins
 	@echo "==> check: green (run 'make check-all' for the full CI set)"
 
 # check-all: the full PR gate. `ci` is the original name and stays —
@@ -132,7 +133,7 @@ GOIMPORTS_VERSION ?= v0.48.0
 # which fails on findings since #430. A green `make ci` must imply a green
 # PR; lint-full's own guard tells you how to install the tool if missing.
 .PHONY: ci
-ci: vet test lint lint-full fmt-check schema-check vulncheck file-budget deadcode check-style check-tool-pins
+ci: vet test lint lint-full fmt-check fmt-selftest schema-check vulncheck file-budget deadcode check-style check-tool-pins
 	@echo "==> ci: all green"
 
 .PHONY: build
@@ -256,29 +257,33 @@ vulncheck:
 lint-full:
 	$(GO) run github.com/golangci/golangci-lint/v2/cmd/golangci-lint@$(GOLANGCI_LINT_VERSION) run
 
+# fmt / fmt-check: gofmt -s (simplification) + goimports -local (import
+# grouping: stdlib / third-party / our own — matches .golangci.yml's
+# local-prefixes).
+#
+# Both scope to `git ls-files '*.go'` rather than `.` (cli#549). `.` is the whole
+# working TREE, so an untracked scratch directory holding Go files — a nested git
+# worktree, a vendored copy, a build sandbox — failed `make check` while every
+# tracked file was clean, and `make fmt` then rewrote content the repo does not
+# track. build.yml's Lint job calls these same targets, so the file set has one
+# definition; see scripts/format.sh for the fail-closed cases.
 .PHONY: fmt
 fmt:
-	gofmt -s -w .
-	$(GO) run golang.org/x/tools/cmd/goimports@$(GOIMPORTS_VERSION) -local github.com/tracebloc/cli -w .
+	@GO="$(GO)" GOIMPORTS_VERSION=$(GOIMPORTS_VERSION) ./scripts/format.sh --write
 
-# fmt-check: gofmt -s (simplification) + goimports -local (import grouping:
-# stdlib / third-party / our own — matches .golangci.yml's local-prefixes).
 .PHONY: fmt-check
 fmt-check:
-	@diff="$$(gofmt -s -l . 2>/dev/null)"; \
-	if [ -n "$$diff" ]; then \
- echo "==> gofmt -s needed on:"; \
- echo "$$diff" | sed 's/^/ /'; \
- echo "==> run \`make fmt\` to fix"; \
- exit 1; \
-	fi
-	@drift="$$($(GO) run golang.org/x/tools/cmd/goimports@$(GOIMPORTS_VERSION) -local github.com/tracebloc/cli -l .)"; \
-	if [ -n "$$drift" ]; then \
- echo "==> goimports (import grouping) needed on:"; \
- echo "$$drift" | sed 's/^/ /'; \
- echo "==> run \`make fmt\` to fix"; \
- exit 1; \
-	fi
+	@GO="$(GO)" GOIMPORTS_VERSION=$(GOIMPORTS_VERSION) ./scripts/format.sh --check
+
+# fmt-selftest: the properties scripts/format.sh must not lose — the formatters
+# are stubbed, so it is hermetic and ~6 s. It exists because the FIRST cut of
+# format.sh shipped a false green: run_formatter `exit`ed from inside a command
+# substitution, which ends only the subshell, so check mode read an empty capture
+# and printed "clean" on a formatter that never ran (caught in review on #550).
+# A comment cannot hold that shut; this can.
+.PHONY: fmt-selftest
+fmt-selftest:
+	@bash scripts/tests/format-verify.sh
 
 .PHONY: schema-check
 schema-check:

scripts/check-tool-pins.sh

@@ -32,6 +32,10 @@ cd "$(dirname "$0")/.." || exit 2
 # Add a row when a tool moves to a `make` target that CI calls.
 TOOLS=(
  "GOVULNCHECK_VERSION:golang.org/x/vuln/cmd/govulncheck"
+ # cli#549: the Lint job's two inline formatter steps became `make fmt-check`,
+ # so build.yml no longer holds its own goimports version. This row is what
+ # keeps that true on the next bump.
+ "GOIMPORTS_VERSION:golang.org/x/tools/cmd/goimports"
 )
 
 fail=0

scripts/format.sh

@@ -0,0 +1,150 @@
+#!/usr/bin/env bash
+# =============================================================================
+# format.sh — the gofmt -s + goimports gate, scoped to TRACKED files (cli#549)
+#
+# Both formatters used to run over `.`, which is the whole working TREE, not the
+# repo. `.` includes untracked directories, so any scratch path holding Go files
+# — a nested git worktree, a vendored copy, a build sandbox — was reported as
+# drift while every tracked file was correctly formatted:
+#
+# ==> goimports (import grouping) needed on:
+# <untracked-scratch-dir>/internal/cli/data.go
+# ==> run `make fmt` to fix
+#
+# CI never saw it (a fresh checkout has no untracked Go files), so it was a
+# local-only FALSE failure — and the remedy it printed, `make fmt`, was the
+# same bug in write mode: it rewrote files the repo does not track.
+#
+# The file set is now `git ls-files '*.go'`: exactly what a PR can contain, so
+# local and CI cannot disagree about scope. Both call this script.
+#
+# Usage:
+# scripts/format.sh --check report drift, exit 1 if any (make fmt-check)
+# scripts/format.sh --write rewrite in place (make fmt)
+#
+# GO and GOIMPORTS_VERSION come from the environment; the Makefile passes them,
+# keeping the version declared once there (backend#1972, check-tool-pins.sh).
+#
+# FAILS CLOSED (exit 2), because each of these would otherwise be reported as a
+# clean pass — the inert-verification class of backend#1729:
+# * not inside a git work tree — no way to know what is tracked
+# * an EMPTY file list — this module has Go files by construction, so "none
+# found" means the query broke, not that the tree is clean. It also matters
+# mechanically: bare `gofmt -l` with no path arguments reads STDIN, so an
+# unguarded empty list checks nothing and exits 0.
+#
+# Portable to bash 3.2 (macOS default): no mapfile, no associative arrays.
+# =============================================================================
+set -uo pipefail
+cd "$(dirname "$0")/.." || exit 2
+
+GO="${GO:-go}"
+GOIMPORTS_VERSION="${GOIMPORTS_VERSION:-v0.48.0}"
+LOCAL_PREFIX="github.com/tracebloc/cli"
+
+mode=""
+case "${1-}" in
+ --check) mode="check" ;;
+ --write) mode="write" ;;
+ *)
+ echo "usage: scripts/format.sh --check | --write" >&2
+ exit 2
+ ;;
+esac
+
+git rev-parse --is-inside-work-tree >/dev/null 2>&1 || {
+ echo "format.sh: not inside a git work tree — cannot tell which files are tracked," >&2
+ echo " and formatting the whole directory instead is the bug this replaced." >&2
+ exit 2
+}
+
+# Tracked .go files that EXIST. `git ls-files` reports index entries, so a
+# tracked-then-deleted file is still listed; passing it to gofmt is a hard error
+# ("no such file or directory") on a tree that is merely mid-edit.
+files=()
+while IFS= read -r -d '' f; do
+ [ -f "$f" ] && files+=("$f")
+done < <(git ls-files -z -- '*.go')
+
+if [ ${#files[@]} -eq 0 ]; then
+ echo "format.sh: git ls-files '*.go' matched no existing tracked file." >&2
+ echo " This module has Go files by construction, so that is a broken query," >&2
+ echo " not a clean tree. Refusing to report clean." >&2
+ exit 2
+fi
+
+# The formatter's stdout goes to a FILE and run_formatter RETURNS the status;
+# it never `exit`s and is never called inside `$( )`. That shape is deliberate and
+# it is load-bearing (Bugbot High + @LukasWodka on #550): a function that `exit`s
+# from a command substitution ends only the SUBSHELL, so the caller reads an empty
+# capture, finds no drift, and prints "clean" — exit 0 on a formatter that never
+# ran. Which is precisely the inert-verification failure (backend#1729) this file
+# was written to prevent, so it must not be reintroduced here.
+#
+# scripts/tests/format-verify.sh asserts the propagation with a formatter stubbed
+# to fail; keep that harness green rather than trusting this comment.
+out_file=""
+cleanup() { [ -n "$out_file" ] && rm -f "$out_file"; }
+trap cleanup EXIT
+
+out_file="$(mktemp "${TMPDIR:-/tmp}/format-sh.XXXXXX")" || {
+ echo "format.sh: mktemp failed — refusing to report clean" >&2
+ exit 2
+}
+
+# xargs, not a bare expansion: the list grows with the repo and a single argv has
+# a hard size limit. printf is a builtin, so building the NUL stream is not itself
+# subject to that limit. The list is non-empty (guarded above), so the BSD-vs-GNU
+# "run once with no arguments" difference cannot bite.
+#
+# stderr is deliberately NOT redirected. `go run` writes module-download progress
+# there, and folding it into the captured stdout would turn a cold cache into
+# phantom "drift" filenames. Diagnostics go straight to the terminal instead.
+run_formatter() { # run_formatter <label> <cmd...> -> 0 ok, 2 the tool failed
+ local label="$1"; shift
+ local rc
+ printf '%s\0' "${files[@]}" | xargs -0 "$@" > "$out_file"
+ rc=$?
+ if [ "$rc" -ne 0 ]; then
+ echo "==> ${label}: FAILED (exit ${rc}) — see the output above" >&2
+ return 2
+ fi
+ return 0
+}
+
+goimports_cmd=(
+ "$GO" run "golang.org/x/tools/cmd/goimports@${GOIMPORTS_VERSION}"
+ -local "$LOCAL_PREFIX"
+)
+
+if [ "$mode" = "write" ]; then
+ run_formatter "gofmt -s -w" gofmt -s -w || exit 2
+ run_formatter "goimports -w" "${goimports_cmd[@]}" -w || exit 2
+ echo "==> fmt: ${#files[@]} tracked Go file(s) formatted"
+ exit 0
+fi
+
+fail=0
+
+report() { # report <heading> — print the drift file, if any, and set fail
+ if [ -s "$out_file" ]; then
+ echo "==> $1"
+ sed 's/^/ /' "$out_file"
+ fail=1
+ fi
+}
+
+run_formatter "gofmt -s" gofmt -s -l || exit 2
+report "gofmt -s needed on:"
+
+# goimports -local: the stdlib / third-party / our-own import grouping that
+# .golangci.yml's local-prefixes already declares. gofmt does not check grouping.
+run_formatter "goimports" "${goimports_cmd[@]}" -l || exit 2
+report "goimports (import grouping) needed on:"
+
+if [ "$fail" -ne 0 ]; then
+ echo "==> run \`make fmt\` to fix"
+ exit 1
+fi
+
+echo "==> fmt-check: ${#files[@]} tracked Go file(s) clean"