diff --git a/.cursor/BUGBOT.md b/.cursor/BUGBOT.md index 9e6d70cd..d53cce8c 100644 --- a/.cursor/BUGBOT.md +++ b/.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 diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 29f65106..3c6110ba 100644 --- a/.github/workflows/build.yml +++ b/.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: | diff --git a/Makefile b/Makefile index 16003549..89d8fdcc 100644 --- a/Makefile +++ b/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: 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..45583810 --- /dev/null +++ b/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: +# /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