From bf70404828f718b8efab57c6c143d54493f75baa Mon Sep 17 00:00:00 2001 From: Asad Iqbal Date: Fri, 21 Aug 2026 18:04:36 +0500 Subject: [PATCH 1/3] fix(fmt): the formatter gates walked the working tree, not the repo (cli#549) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `make fmt-check` ran `gofmt -s -l .` and `goimports -l .`, and `.` is the whole working TREE. Any untracked directory holding Go files — a nested git worktree, a vendored copy, a build sandbox — was reported as drift while every tracked file in the repo was correctly formatted: ==> goimports (import grouping) needed on: /internal/cli/data.go ==> run `make fmt` to fix CI never saw it, because a fresh checkout has no untracked Go files. So this was a local-only FALSE failure in `make check`, the documented pre-push tier — and the remedy it printed was the same bug in write mode: `make fmt` (`gofmt -s -w .`) rewrote files the repo does not track, i.e. someone else's working copy. Both invocations, check and write, now take the file list from `git ls-files '*.go'` — exactly the set a PR can contain. Notably this was the ONLY gate affected: Go's `./...` skips dot-prefixed directories, so `vet`, `test` and `deadcode` never saw `.claude/` at all. `gofmt .` does not skip them, which is why fmt-check alone cried wolf. scripts/format.sh, rather than more backslash-continued shell in the Makefile, because the edge cases want testing and comments: * xargs over a NUL stream from a printf builtin, not a bare argv expansion. Measured: 9000 tracked files is 2.0 MB of argv against a 1 MB ARG_MAX — the naive form dies with "argument list too long", the batched form reports all 9000. * tracked-but-DELETED index entries are filtered out. `git ls-files` reports the index, so a mid-edit deletion would otherwise hard-error the gate. * FAILS CLOSED (exit 2) outside a git work tree, and on an empty file list. The empty case is not cosmetic: bare `gofmt -l` with no path arguments reads STDIN, so an unguarded empty list checks nothing and exits 0 — the inert-verification class of backend#1729. * stderr is deliberately not captured; `go run`'s download progress would otherwise appear as phantom drift filenames on a cold cache. build.yml's Lint job calls `make fmt-check` instead of keeping its own inline copy, so the file set has one definition and cannot drift from local. That also drops the restated `goimports@v0.48.0` pin, and GOIMPORTS_VERSION joins check-tool-pins.sh's TOOLS so it stays dropped on the next bump. Verified on this branch, with a `.claude/worktrees/` directory present holding deliberately misformatted Go: * `make check` — green (was red on the same tree before this change; the old `gofmt -s -l .` flags the scratch file, shown in the ticket) * tracked drift still caught, both gates: a non-simplified slice expression in internal/slug/slug.go fails gofmt -s, a mis-grouped import fails goimports * `make fmt` leaves the scratch file byte-identical (sha unchanged) * fail-closed paths exercised: no work tree, empty list, bad usage — all 2 * a deleted tracked file passes at 218/219 rather than erroring * shellcheck (default severity, not just -error) clean; actionlint clean Refs cli#549. Found while working on cli#548, kept out of it to keep that PR scoped to the offboard telemetry bug (backend#2314). --- .cursor/BUGBOT.md | 5 ++ .github/workflows/build.yml | 36 ++++------ Makefile | 30 ++++----- scripts/check-tool-pins.sh | 4 ++ scripts/format.sh | 129 ++++++++++++++++++++++++++++++++++++ 5 files changed, 163 insertions(+), 41 deletions(-) create mode 100755 scripts/format.sh diff --git a/.cursor/BUGBOT.md b/.cursor/BUGBOT.md index 9e6d70cd..b69bff0b 100644 --- a/.cursor/BUGBOT.md +++ b/.cursor/BUGBOT.md @@ -125,6 +125,11 @@ 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. - **`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 diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 29f65106..1b5f71a4 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -77,6 +77,7 @@ 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 dash -n scripts/install.sh bash -n scripts/tests/install-verify.sh shellcheck --shell=bash --severity=error scripts/tests/install-ps1-verify.sh @@ -152,29 +153,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: | diff --git a/Makefile b/Makefile index 16003549..9c385d46 100644 --- a/Makefile +++ b/Makefile @@ -256,29 +256,23 @@ 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 .PHONY: schema-check schema-check: diff --git a/scripts/check-tool-pins.sh b/scripts/check-tool-pins.sh index 778b1d2d..f5e4ae8a 100755 --- a/scripts/check-tool-pins.sh +++ b/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 diff --git a/scripts/format.sh b/scripts/format.sh new file mode 100755 index 00000000..094cfb67 --- /dev/null +++ b/scripts/format.sh @@ -0,0 +1,129 @@ +#!/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: +# /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 + +# 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 captured. `go run` writes module-download progress +# there, and folding that into the captured stdout would turn a cold cache into +# phantom "drift" filenames. Diagnostics go straight to the terminal instead. +run_formatter() { # run_formatter