diff --git a/.cursor/BUGBOT.md b/.cursor/BUGBOT.md index 261320b..81dd02b 100644 --- a/.cursor/BUGBOT.md +++ b/.cursor/BUGBOT.md @@ -13,8 +13,10 @@ Two things make this repo unusual and should shape every finding: 1. **Its exit codes are a scripting contract** — customers branch on them (`internal/cli/exitcodes.go`: "the numeric values are FROZEN"). 2. **`make ci` mirrors CI exactly.** The `Makefile` header states it outright: "divergence - between local and CI is the bug this file exists to prevent." Tool versions are pinned in - lockstep with `.github/workflows/build.yml`. + between local and CI is the bug this file exists to prevent." Tool versions are declared + once, in the `Makefile`; the workflows call `make lint` / `make fmt-check` / + `make vulncheck` and `golangci.yml` reads its pin via `make print-GOLANGCI_LINT_VERSION`. + `scripts/check-tool-pins.sh` fails the Lint job if a workflow restates one. ## Always flag @@ -128,11 +130,18 @@ Two things make this repo unusual and should shape every finding: ## Known non-issues — do not flag -- **`.golangci.yml` does not gate CI.** `golangci-lint` is never invoked in a workflow (its - `staticcheck`/`unused` are disabled there for runner OOM reasons); the blocking Lint job runs - 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. +- **`.golangci.yml` is one of two lint gates, not the only one.** `golangci.yml` runs it + (blocking exit-code check `golangci-lint`; `staticcheck`/`unused` are disabled there for + runner OOM reasons). The blocking Lint job in `build.yml` runs the standalone set via + `make lint` — `errcheck`, `ineffassign`, `misspell`, `staticcheck` — plus `make fmt-check` + (`gofmt -s`, `goimports`), `deadcode-check.sh`, `file-budget.sh`, `check-style.sh`, + `check-tool-pins.sh`. + Don't infer the full lint coverage from either file alone. +- **`build.yml`'s Build job is three legs (linux/amd64 + windows/amd64 + darwin/arm64) on + purpose.** The 8-target cross-compile lives only in `release.yml`; do not flag the smaller + matrix as a missing platform, and do not propose re-adding legs "to keep in lock-step" — + there is deliberately no second copy. The job comment names what each of the three tells + us per commit; read it before proposing a fourth. - **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 diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 7265b0e..9d52d01 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -1,9 +1,9 @@ name: Build # Runs on every PR + every push to develop/main. Validates the binary -# builds cleanly across all 8 release-target platforms, the tests pass, -# and the linter is green. Release-time signing + tag artifacts live -# in release.yml (Phase 5). +# builds on the three platforms that tell us something per commit (see the +# `build` job), the tests pass, and the linter is green. The full 8-target +# cross-compile, signing and tag artifacts live in release.yml (Phase 5). on: push: @@ -77,6 +77,8 @@ 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/tests/tool-pins-verify.sh + bash -n scripts/tests/tool-pins-verify.sh shellcheck --shell=bash --severity=error scripts/format.sh shellcheck --shell=bash --severity=error scripts/tests/format-verify.sh shellcheck --shell=bash --severity=error scripts/cosign-retry.sh @@ -97,6 +99,12 @@ jobs: # "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 + # check-tool-pins.sh's own properties: both restatement shapes redden + # (`module@version` in a run step, a literal `version:` on the tool's + # action), an unrelated action's `version:` does not, and missing inputs + # fail closed. Hermetic — the real guard runs against a fixture tree. + - name: Tool-pin guard harness (both shapes redden / fail-closed) + run: bash scripts/tests/tool-pins-verify.sh - name: Verification harness (mandatory cosign / fail-closed) run: bash scripts/tests/install-verify.sh @@ -170,18 +178,20 @@ jobs: # against the same code in <30 seconds and catch the bugs # we've actually hit this week (unchecked Fprintf errors # were errcheck-catchable; gofmt -s drift was a recurring - # nit). Going back to the action when #6 has a strategy. - - # Tool versions are pinned for reproducibility (was @latest, which - # drifted — see #127). The current tags build cleanly against this - # module's Go floor; the old "stale x/tools breaks the build" worry - # is gone now that these tools ship newer tags. Keep these in lockstep - # with the *_VERSION vars in the Makefile. Bump deliberately. - - - name: errcheck - run: | - go install github.com/kisielk/errcheck@v1.20.0 - errcheck ./... + # nit). golangci.yml runs the non-SSA linters through the + # action separately; staticcheck stays standalone here. + + # `make lint`, not four inline `go install tool@version` steps. errcheck, + # ineffassign, misspell and staticcheck (`-checks all,-ST1005` — ST1005 + # flags ~58 customer-visible error strings that need a deliberate wording + # review, follow-up to #279) each had a second copy of their version here, + # held in step with the Makefile's *_VERSION vars by a comment asking + # people to remember (#127). The Makefile is now the only declaration and + # scripts/check-tool-pins.sh reddens this job if a pin creeps back in — + # the same move cli#549 made for the formatters below and the govulncheck + # job made for its tool. + - name: errcheck + ineffassign + misspell + staticcheck + run: make lint # gofmt -s (simplification) + goimports -local (the stdlib / third-party / # our-own import grouping that .golangci.yml's local-prefixes declares; @@ -196,26 +206,6 @@ jobs: - name: gofmt -s + goimports -local run: make fmt-check - - name: ineffassign - run: | - go install github.com/gordonklaus/ineffassign@v0.2.0 - ineffassign ./... - - - name: misspell - run: | - go install github.com/client9/misspell/cmd/misspell@v0.3.4 - misspell -error . - - # staticcheck (standalone, pinned): the full `all` suite minus ST1005. - # ST1005 (error-string style) flags ~58 customer-visible error strings - # that need a deliberate wording review — follow-up to #279, not a - # mechanical sweep. The old golangci-lint OOM story does not apply to - # the standalone binary: full run is ~12s wall on this module. - - name: staticcheck - run: | - go install honnef.co/go/tools/cmd/staticcheck@2025.1.1 - staticcheck -checks all,-ST1005 ./... - # deadcode: BLOCKING reachability scan from the CLI entrypoint (~5s). # The four legit unreachables — Stringer methods (Status.String, # JobOutcome.String) reached only via fmt reflection that static @@ -278,32 +268,39 @@ jobs: build: timeout-minutes: 20 name: Build (${{ matrix.os }}/${{ matrix.arch }}) + # Three legs, not the eight release targets. This job is advisory (it is not a + # required check) and the other five legs were most of a 920 s run — 55 % of + # every push and PR — for compile-only confirmation of platforms that + # release.yml builds, signs and publishes anyway at every tag. What is kept + # is what tells us something per commit: + # + # linux/amd64 — the only leg whose binary can RUN on the runner, so it + # carries the smoke test below. + # windows/amd64 — the one target with its own code path: x/sys/windows + # and the OS-conditional parts of client-go compile only + # under GOOS=windows, and a Linux-only dev loop would not + # notice a break there until the release. + # darwin/arm64 — the team's own machines. The per-PR artifact is how a + # reviewer tries a PR's binary locally before it merges; + # release download counts say nothing about that use. + # + # The 8-target matrix has ONE home now, release.yml — there is no second copy + # here to keep in lock-step. A cross-compile break on any other target + # surfaces at the next rc tag, where fail-fast: false shows every broken leg + # in one run and `publish` refuses to ship a partial release. runs-on: ubuntu-latest strategy: - # Don't fail-fast — we want the matrix to surface ALL broken - # platforms in one run, not stop at the first. + # Don't fail-fast — surface every leg's verdict in one run. fail-fast: false matrix: include: - os: linux arch: amd64 - - os: linux - arch: arm64 - - os: linux - arch: '386' - - os: linux - arch: arm - goarm: '6' # keep in lock-step with release.yml's matrix - - os: darwin - arch: amd64 - - os: darwin - arch: arm64 - os: windows arch: amd64 ext: .exe - - os: windows + - os: darwin arch: arm64 - ext: .exe steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 @@ -317,7 +314,6 @@ jobs: env: GOOS: ${{ matrix.os }} GOARCH: ${{ matrix.arch }} - GOARM: ${{ matrix.goarm }} # only applies when GOARCH=arm CGO_ENABLED: "0" run: | mkdir -p dist @@ -333,11 +329,11 @@ jobs: - name: Smoke test (Linux amd64 only — host arch matches runner) if: matrix.os == 'linux' && matrix.arch == 'amd64' - # Cross-built darwin/windows/arm64 binaries can't run on the - # ubuntu runner, so only the native build gets exercised - # post-compile. The other platforms get compile-time - # validation only — which is still meaningful because - # client-go and most k8s deps have OS-conditional code paths. + # The cross-built windows binary can't run on the ubuntu runner, + # so only the native build gets exercised post-compile. The + # windows leg gets compile-time validation only — which is still + # meaningful because client-go and most k8s deps have + # OS-conditional code paths. run: | BIN=./dist/tracebloc-linux-amd64 "$BIN" version diff --git a/.github/workflows/golangci.yml b/.github/workflows/golangci.yml index da4d148..6939927 100644 --- a/.github/workflows/golangci.yml +++ b/.github/workflows/golangci.yml @@ -40,8 +40,8 @@ jobs: # ride the same exit path. History of the advisory era (the # --issues-exit-code=0 flag, why job-level continue-on-error was # not the tool) is in the #423/#426 discussions if you need it. - # Final step of the flip = marking this check required in branch - # protection (backend#1305 / epic #930). + # The flip is complete: `golangci-lint` is listed in develop's required + # status checks (branch protection, read via the API on 2026-09-09). # ================================================================== steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 @@ -52,15 +52,29 @@ jobs: go-version-file: go.mod cache: true - # Both versions pinned for reproducibility, same policy as the - # standalone tools in build.yml (#127). golangci-lint v2.12.2 is - # built with Go 1.26 (required: go.mod says `go 1.26.0`; v1-era - # binaries can't typecheck this module). Bump deliberately, and - # keep the version in step with the format expectations noted in - # .golangci.yml AND with GOLANGCI_LINT_VERSION in the Makefile - # (make lint-full runs the same pinned version -- the local/CI - # mirror depends on the two never drifting). + # The golangci-lint version is NOT restated here. It is declared once, + # as GOLANGCI_LINT_VERSION in the Makefile (`make lint-full` runs that + # same version locally), read by the step below and fed to the action. + # The two used to be held in step by a comment asking people to bump + # both; scripts/check-tool-pins.sh now reddens the Lint job if a + # literal `version:` comes back on this action. The action is kept + # (rather than `make lint-full`) for its result cache and annotations. + # + # Whatever version the Makefile names must be a v2-era build (go.mod + # says `go 1.26.0`; v1-era binaries can't typecheck this module) and + # match the format expectations noted in .golangci.yml. Bump deliberately. + - name: Read GOLANGCI_LINT_VERSION from the Makefile + id: pin + run: | + v="$(make -s print-GOLANGCI_LINT_VERSION)" + if [ -z "$v" ]; then + echo "::error::GOLANGCI_LINT_VERSION is empty in the Makefile — refusing to run an unpinned golangci-lint." + exit 1 + fi + echo "version=$v" >> "$GITHUB_OUTPUT" + echo "golangci-lint $v (from Makefile)" + - name: golangci-lint run (.golangci.yml) uses: golangci/golangci-lint-action@ba0d7d2ec06a0ea1cb5fa41b2e4a3ab91d21278a # v9.3.0 with: - version: v2.12.2 + version: ${{ steps.pin.outputs.version }} diff --git a/Makefile b/Makefile index 89d8fdc..ff44f8f 100644 --- a/Makefile +++ b/Makefile @@ -34,7 +34,7 @@ help: @echo " install go install ./cmd/tracebloc" @echo @echo " individual: vet test lint lint-full fmt fmt-check fmt-selftest" - @echo " schema-check" + @echo " schema-check check-tool-pins tool-pins-selftest" @echo " vulncheck deadcode file-budget check-style clean" @echo " cover cover-integration cover-merge test-integration" @@ -51,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 fmt-selftest file-budget check-style check-tool-pins +check: vet test-fast fmt-check fmt-selftest file-budget check-style check-tool-pins tool-pins-selftest @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 — @@ -116,8 +116,10 @@ GO ?= go PKGS := ./... # Pinned lint/analysis tool versions (reproducibility — no more @latest drift). -# Keep these in lockstep with .github/workflows/build.yml — and -# GOLANGCI_LINT_VERSION with .github/workflows/golangci.yml. Bump deliberately. +# These are the ONLY declarations: build.yml runs `make lint` / `make fmt-check` +# / `make vulncheck`, golangci.yml reads GOLANGCI_LINT_VERSION via +# `make print-GOLANGCI_LINT_VERSION`, and scripts/check-tool-pins.sh reddens the +# Lint job if a workflow restates any of them. Bump here, once, deliberately. GOLANGCI_LINT_VERSION ?= v2.12.2 ERRCHECK_VERSION ?= v1.20.0 INEFFASSIGN_VERSION ?= v0.2.0 @@ -133,7 +135,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 fmt-selftest 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 tool-pins-selftest @echo "==> ci: all green" .PHONY: build @@ -207,10 +209,10 @@ cover-merge: echo "==> overall (unit union integration):"; \ $(GO) tool cover -func=$(COVERDIR)/merged.txt | tail -1 -# Lint set matched to .github/workflows/build.yml's lint job: errcheck + -# ineffassign + misspell + staticcheck (gofmt -s is `fmt-check`, go vet -# is `vet`). CI runs the SAME pinned standalone tools, keeping the -# "make ci green => CI green" invariant this Makefile exists to protect. +# Lint set: errcheck + ineffassign + misspell + staticcheck (gofmt -s is +# `fmt-check`, go vet is `vet`). build.yml's Lint job calls THIS target +# rather than restating the four tools, so the "make ci green => CI green" +# invariant this Makefile exists to protect has one definition to drift from. # `make lint-full` keeps golangci-lint available for a richer local pass. # # staticcheck runs `-checks all,-ST1005`: ST1005 (error-string style) is @@ -244,6 +246,22 @@ deadcode: check-tool-pins: bash scripts/check-tool-pins.sh +# tool-pins-selftest: the properties check-tool-pins.sh must not lose — both +# restatement shapes redden, an unrelated action's `version:` does not, missing +# inputs fail closed. Hermetic (no Go, no network); the real guard runs against +# a fixture tree. Same rationale as fmt-selftest: a guard that stops reddening +# looks exactly like one with nothing to find. +.PHONY: tool-pins-selftest +tool-pins-selftest: + @bash scripts/tests/tool-pins-verify.sh + +# print-: echo one Makefile variable. For a workflow that has to feed a +# pinned version into a GitHub action input (golangci.yml) — it reads the +# declaration instead of holding a copy, which is what lets check-tool-pins.sh +# treat any literal version on that action as a defect. +print-%: + @echo '$($*)' + .PHONY: vulncheck vulncheck: $(GO) run golang.org/x/vuln/cmd/govulncheck@$(GOVULNCHECK_VERSION) ./... diff --git a/VERSION b/VERSION index 211f7aa..dbca4f3 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -0.10.24 +0.10.25 diff --git a/go.mod b/go.mod index 45652b1..176d823 100644 --- a/go.mod +++ b/go.mod @@ -43,7 +43,7 @@ require ( github.com/schollz/progressbar/v3 v3.19.1 github.com/spf13/cobra v1.10.2 github.com/spf13/pflag v1.0.10 - golang.org/x/sys v0.47.0 + golang.org/x/sys v0.48.0 golang.org/x/term v0.45.0 golang.org/x/text v0.41.0 gopkg.in/yaml.v3 v3.0.1 diff --git a/go.sum b/go.sum index 5c01e85..ec281de 100644 --- a/go.sum +++ b/go.sum @@ -149,8 +149,8 @@ golang.org/x/sys v0.0.0-20201119102817-f84b799fce68/go.mod h1:h1NjWce9XRLGQEsW7w golang.org/x/sys v0.0.0-20210615035016-665e8c7367d1/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.0.0-20220520151302-bc2c85ada10a/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.0.0-20220722155257-8c9f86f7a55f/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= -golang.org/x/sys v0.47.0 h1:o7XGOvZQCADBQQ4Y7VNq2dRWQR7JmOUW8Kxx4ZsNgWs= -golang.org/x/sys v0.47.0/go.mod h1:4GL1E5IUh+htKOUEOaiffhrAeqysfVGipDYzABqnCmw= +golang.org/x/sys v0.48.0 h1:bbX/i/6MgT9BVLM9RT1thmxL04yeTAhbEz4SyadbXoo= +golang.org/x/sys v0.48.0/go.mod h1:hNLxWAXmnKAxqDtdwIYC4bM9oQPEecfsnNMuSxOs3og= golang.org/x/term v0.0.0-20201126162022-7de9c90e9dd1/go.mod h1:bj7SfCRtBDWHUb9snDiAeCFNEtKQo2Wmx5Cou7ajbmo= golang.org/x/term v0.0.0-20210927222741-03fcf44c2211/go.mod h1:jbD1KX2456YbFQfuXm/mYQcufACuNUgVhRMnK/tPxf8= golang.org/x/term v0.45.0 h1:NwWyBmoJCbfTHpxrWoZ9C6/VxOf7ic219I8xZZFdrf0= diff --git a/internal/resources/resources.go b/internal/resources/resources.go index e54f9d6..3a6bcee 100644 --- a/internal/resources/resources.go +++ b/internal/resources/resources.go @@ -13,9 +13,11 @@ // training pod comes out Guaranteed. Two caveats this comment used to elide // (backend#2872): the built-in fallback is the contract floor // cpu=1,memory=2Gi since backend#2254, not the "cpu=2,memory=8Gi" named -// here before; and a GPU pod is BestEffort whatever this writes, because -// client-runtime's GPU path sets only nvidia.com/gpu and ephemeral-storage -// and neither counts toward QoS (backend#2871). This is the +// here before; and a GPU pod does not carry this value as written: once +// client-runtime bounds it, it carries this envelope TIMES its nvidia.com/gpu +// limit (see DefaultTraining), and until then it is BestEffort, because the +// GPU path set only nvidia.com/gpu and ephemeral-storage and neither counts +// toward QoS (backend#2871). For a CPU pod this is the // exact value client-runtime's jobs_manager.py stamps on spawned jobs and // the same value `cluster doctor`'s checkNodeFit already parses — so the two // read it identically (di#358 lesson: a reader must mirror the writer). @@ -48,18 +50,29 @@ import ( // in exactly one place (the contract), and nothing should be able to shadow it // with a second literal. // -// SIZED FOR ONE RANK, and nothing divides it (backend#2543). Recorded here -// because this is where the number lives and the constraint is invisible from -// the value. Under replication (TRACEBLOC_DDP, backend#2224) N ranks run as N -// processes inside ONE pod sharing this envelope, and its two halves are treated -// differently: the CPU quota IS divided across ranks (tracebloc-engine#732 sizes -// the thread pools from quota/world_size), memory is NOT divided by anything. +// SIZED FOR ONE RANK (backend#2543). Recorded here because this is where the +// number lives and the constraint is invisible from the value. Under +// replication (TRACEBLOC_DDP, backend#2224) N ranks run as N processes inside +// ONE pod, and every rank holds a full replica -- parameters, its own gradients, +// its own optimizer state -- so what the pod needs grows with N while this value +// does not. Nothing here divides or multiplies it; that is the runtime's job, +// and it does it differently per pod class: // -// So the floor is correct for world_size == 1 and quietly wrong above it -- and -// not by a uniform factor, which is why no single scaling of "the envelope" fixes -// it. This is a STATEMENT, not a fix: per-job vs per-rank is backend#2543's open -// question and belongs with whoever owns sizing. Replication is default-off, so -// nothing is wrong today. +// - A CPU pod carries exactly this value. It runs the CPU image, whose torch +// has no CUDA, so its world_size is always 1 and the question never binds. +// - A GPU pod's rank count is its nvidia.com/gpu limit, and client-runtime's +// jobs_manager.py stamps THIS PER-RANK BASE TIMES THAT COUNT, capped so the +// node's GPU pods still pack, and refuses admission on a single-node edge +// when the scaled envelope can never be placed (client-runtime#483). +// Unconditional for limits above one; a single-GPU pod is bounded only +// behind the runtime's GPU_POD_ENVELOPE opt-in and is BestEffort otherwise. +// - The CPU quota is then divided the other way inside the pod: +// tracebloc-engine#732 sizes each rank's thread pools from +// quota/world_size. Consistent, not contradictory -- N times one rank's +// cpu, split N ways, is one rank's cpu each. +// +// So the number `show` reports is the per-rank base a CPU run carries as-is, +// not what a multi-GPU pod is stamped with. func DefaultTraining() string { f := mustContract().Floor cpu := resource.NewMilliQuantity(f.CPUMilli, resource.DecimalSI) diff --git a/scripts/check-tool-pins.sh b/scripts/check-tool-pins.sh index f5e4ae8..c04bff6 100755 --- a/scripts/check-tool-pins.sh +++ b/scripts/check-tool-pins.sh @@ -19,6 +19,8 @@ # Runs in CI (the Lint job, beside check-style.sh) and locally: # make check-tool-pins (or: bash scripts/check-tool-pins.sh) # Exit 0 = clean, 1 = a restated pin was found, 2 = the guard itself errored. +# Its own properties (reddens on each restatement shape, fails closed on missing +# inputs) are pinned by scripts/tests/tool-pins-verify.sh (make tool-pins-selftest). # ============================================================================= set -uo pipefail cd "$(dirname "$0")/.." || exit 2 @@ -28,22 +30,57 @@ cd "$(dirname "$0")/.." || exit 2 [[ -f Makefile ]] || { echo "check-tool-pins: no Makefile — refusing to report clean" >&2; exit 2; } [[ -d .github/workflows ]] || { echo "check-tool-pins: no .github/workflows — refusing to report clean" >&2; exit 2; } -# Tools whose version the Makefile owns, as :. -# Add a row when a tool moves to a `make` target that CI calls. +# Tools whose version the Makefile owns, as +# +# :[:] +# +# Add a row when a tool moves to a `make` target that CI calls. Two restatement +# shapes are caught: +# +# 1. `@` anywhere in a workflow — the `go install` / `go run` +# form. Every row is checked for this. +# 2. A literal `version:` input under a `uses: @...` step — the form a +# tool takes when a workflow runs it through its GitHub action instead of +# `go run`. Only rows with the optional third field are checked for this; +# a `version: ${{ steps..outputs. }}` that reads the Makefile +# (`make print-`) is not a restatement and passes. 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" + # The Lint job's four inline `go install tool@version` steps became one + # `make lint`. These rows are what keep the Makefile the only declaration. + "ERRCHECK_VERSION:github.com/kisielk/errcheck" + "INEFFASSIGN_VERSION:github.com/gordonklaus/ineffassign" + "MISSPELL_VERSION:github.com/client9/misspell/cmd/misspell" + "STATICCHECK_VERSION:honnef.co/go/tools/cmd/staticcheck" + # golangci.yml runs golangci-lint through its action, so the restatement to + # catch is the action's `version:` input (shape 2), not `module@version`. + "GOLANGCI_LINT_VERSION:github.com/golangci/golangci-lint/v2/cmd/golangci-lint:golangci/golangci-lint-action" ) +# The workflow files, listed once. An empty list is a malfunction, not a pass: +# with nothing to scan, "no offender found" is the unearned green. +workflows=() +while IFS= read -r -d '' f; do workflows+=("$f"); done < <( + find .github/workflows -maxdepth 1 -type f \( -name '*.yml' -o -name '*.yaml' \) -print0 | sort -z +) +if (( ${#workflows[@]} == 0 )); then + echo "check-tool-pins: no workflow files under .github/workflows — refusing to report clean" >&2 + exit 2 +fi + fail=0 checked=0 for row in "${TOOLS[@]}"; do var="${row%%:*}" - module="${row#*:}" + rest="${row#*:}" + module="${rest%%:*}" + action="" + [[ "$rest" == *:* ]] && action="${rest#*:}" # Parse the REAL declaration. `?=` or `=`, any surrounding spaces. version="$(sed -nE "s/^[[:space:]]*${var}[[:space:]]*\\??=[[:space:]]*([^[:space:]#]+).*/\\1/p" Makefile | head -1)" @@ -78,6 +115,51 @@ for row in "${TOOLS[@]}"; do echo " Call the make target instead." >&2 fail=1 fi + + # Shape 2: a literal `version:` input on the tool's GitHub action. The state + # machine is one step wide and indentation-aware: + # + # - a `uses:` of the action ARMS it and records the column of the `uses:` + # key (the same column for `- uses:` and for `uses:` under `- name:`, + # which is where the step's sibling keys `with:`/`id:`/`name:` sit); + # - any later non-blank, non-comment line SHALLOWER than that column is a + # sibling step (`- name:` / `- uses:`) or a parent key and DISARMS it. + # YAML list items inside the step's own `with:` block (`args:` items) are + # deeper and do not — disarming on any `- ` line let a literal `version:` + # after such a list pass clean; + # - state resets at every file boundary (FNR == 1), so a file that ends on + # the action's `uses:` line cannot arm the next file in sort order; + # - while armed, a `version:` whose value is not a `${{ ... }}` expression + # is a copy — any literal, not just a digit-leading one. `latest` is both + # an un-pinning and a restatement the action honors. + if [[ -n "$action" ]]; then + action_offenders="$(awk -v action="$action" ' + FNR == 1 { armed = 0 } + /^[[:space:]]*#/ || /^[[:space:]]*$/ { next } + { match($0, /^[[:space:]]*/); ind = RLENGTH } + armed && ind < key { armed = 0 } + index($0, "uses:") && index($0, action "@") { armed = 1; key = index($0, "uses:") - 1; next } + armed && /^[[:space:]]*version:/ { + v = $0 + sub(/^[[:space:]]*version:[[:space:]]*["]?/, "", v) + if (index(v, "${{") != 1) printf "%s:%d:%s\n", FILENAME, FNR, $0 + } + ' "${workflows[@]}")" + rc=$? + if (( rc != 0 )); then + echo "check-tool-pins: awk errored (rc=${rc}) scanning for a literal version: on ${action} — refusing to report clean" >&2 + exit 2 + fi + if [[ -n "$action_offenders" ]]; then + echo "A workflow gives ${action} a literal version:" >&2 + printf '%s\n' "$action_offenders" >&2 + echo >&2 + echo " ${var} in the Makefile already declares this (${version}). Read it in a step" >&2 + echo " (make print-${var}) and pass \${{ steps..outputs.version }} instead, so the" >&2 + echo " action and make lint-full can never run different versions." >&2 + fail=1 + fi + fi checked=$((checked + 1)) done diff --git a/scripts/tests/tool-pins-verify.sh b/scripts/tests/tool-pins-verify.sh new file mode 100755 index 0000000..771fabc --- /dev/null +++ b/scripts/tests/tool-pins-verify.sh @@ -0,0 +1,222 @@ +#!/usr/bin/env bash +# ============================================================================= +# tool-pins-verify.sh — the properties scripts/check-tool-pins.sh must not lose +# +# The guard exists to turn "keep these versions in lockstep" comments into a +# check. A guard that stops reddening is indistinguishable from one that has +# nothing to find, so this harness asserts, against the REAL script (copied +# verbatim into a fixture tree — never a re-implementation of its rules): +# +# 1. clean tree -> exit 0 +# 2. `@` restated in a workflow -> exit 1, names the file +# 3. literal `version:` on the tool's action -> exit 1, names the file +# 4. literal `version:` on an UNRELATED action -> exit 0 (no false positive) +# 5. Makefile missing a declared tool -> exit 2 (fail closed) +# 6. no workflow files at all -> exit 2 (fail closed) +# 7. a YAML list inside the action's own `with:` before a literal `version:` +# (e.g. `args:` items) -> exit 1 (list items do not disarm) +# 8. one file ends on the action's `uses:` line, the next begins with a +# `version:` line -> exit 0 (state resets per file) +# 9. `version: latest` on the tool's action -> exit 1 (any literal is a copy, +# not just digit-leading values) +# +# The tool rows are DERIVED from the guard's own TOOLS array, so this file holds +# no module path or make variable of its own to drift from it. +# Hermetic: no Go, no network, temp dir only. +# +# Runs in CI (the Installer job, beside the other *-verify.sh harnesses) and +# locally via `make tool-pins-selftest`. Exit 0 = all properties hold. +# ============================================================================= +set -uo pipefail + +REPO="$(cd "$(dirname "$0")/../.." && pwd)" +GUARD="$REPO/scripts/check-tool-pins.sh" +[[ -f "$GUARD" ]] || { echo "tool-pins-verify: $GUARD not found" >&2; exit 2; } + +# Derive the rows from the guard: every quoted ":[:]" inside +# the TOOLS=( ... ) block. Zero rows means the parse broke — fail, don't pass. +rows=() +while IFS= read -r r; do rows+=("$r"); done < <( + sed -n '/^TOOLS=(/,/^)/p' "$GUARD" | sed -nE 's/^[[:space:]]*"([A-Z_]+:[^"]+)".*/\1/p' +) +(( ${#rows[@]} > 0 )) || { echo "tool-pins-verify: parsed 0 TOOLS rows from the guard — refusing to report clean" >&2; exit 2; } + +module_row="" # first row with no action field (shape 1) +action_row="" # first row with an action field (shape 2) +for r in "${rows[@]}"; do + rest="${r#*:}" + if [[ "$rest" == *:* ]]; then + [[ -z "$action_row" ]] && action_row="$r" + else + [[ -z "$module_row" ]] && module_row="$r" + fi +done +[[ -n "$module_row" && -n "$action_row" ]] || { echo "tool-pins-verify: need one module-only row and one action row in TOOLS to exercise both shapes" >&2; exit 2; } + +tmp="$(mktemp -d)" +trap 'rm -rf "$tmp"' EXIT +fails=0 +passes=0 + +# fixture: a fresh tree with the real guard at scripts/, a Makefile declaring +# every row, and an empty workflows dir. Callers append workflow files. +fixture() { + rm -rf "$tmp/repo" + mkdir -p "$tmp/repo/scripts" "$tmp/repo/.github/workflows" + cp "$GUARD" "$tmp/repo/scripts/check-tool-pins.sh" + : > "$tmp/repo/Makefile" + local r + for r in "${rows[@]}"; do + printf '%s ?= v9.9.9-fixture\n' "${r%%:*}" >> "$tmp/repo/Makefile" + done +} + +# run