Lint in a container as a build step, via Dockerfile.lint (closes #113)

Every lint run now happens inside its own container, invoked through
script/lint, and linting is a build step rather than a container
command: a successful build of the new root Dockerfile.lint IS a clean
lint. That shape also works where the docker daemon is remote and bind
mounts are impossible.

Its FROM line -- golangci/golangci-lint:v2.12.2, pinned by digest -- is
now the only pin of the linter version in this repo.

A container per run has its own lint cache and its own golangci-lint
lock, both discarded with it, so neither cross-worktree contamination
nor lock contention exists any more. The machinery that defended
against them is therefore gone: the per-worktree cache directories, the
lock-retry loop, and script/lint-audit, which existed to catch findings
replayed from a cache that no longer exists. So is the host lint path
in its entirety -- the native escape hatch, its version detection, and
VAULTIK_LINT_IN_CONTAINER in both script/lint and the Dockerfile.
Nothing lints on the host, at any version.

A cached build lints nothing, so the CHECK_EPOCH mechanism the product
Dockerfile already used is what makes a green mean something:
ARG CHECK_EPOCH with no default, placed below the module layers so
dependency caching survives, a `RUN [ -n "$CHECK_EPOCH" ] || exit 1`
guard so a build that withholds the arg fails instead of replaying, and
the value expanded into the lint command itself. script/lint computes
`epoch="$(date +%s%N)$$"` as a bare assignment on its own line, because
inline in the argument a failing substitution does not abort under
`set -eu` and yields a constant empty epoch -- which is exactly the
false green being prevented.

The product Dockerfile loses its lint stage rather than gaining a
second linter pin. That stage ran `make lint`, which is now
`docker build`: docker-in-docker inside a BuildKit step with no daemon.
Calling golangci-lint directly there instead would have meant two
independently bumpable digests for one tool. `make fmt-check` moves
beside `make test` in the builder stage, and script/cibuild now builds
Dockerfile.lint and then Dockerfile, each with its own fresh epoch,
failing on either. Consequence, stated in comments rather than left to
be discovered: script/docker builds the product image only and no
longer lints; script/check and script/cibuild are the gates.

`golangci-lint config verify` runs as its own epoch-keyed layer, above
the lint. It is not belt-and-braces: `golangci-lint run` rejects a
config it cannot PARSE but silently IGNORES an unknown top-level KEY.
Renaming .golangci.yml's `linters:` to `linterz:` -- one character --
discards `default: all`, the disable list and every threshold, leaves
only the small default linter set running, and exits 0 reporting
`0 issues.` on a tree the real config fails with an lll finding, in a
run whose lint layer demonstrably executed. That is a set-but-
ineffective config falling back to defaults instead of failing loudly,
sitting in the gate's own configuration. `config verify` catches it and
does so with the network genuinely off at this pin: under
`docker run --network none` against the pinned digest it exits 0 on
this repo's config and exits 3 on the `linterz:` variant. It is keyed
on CHECK_EPOCH like the lint itself, because a cached validation
validates nothing.

script/lint-fix is kept, reimplemented as a bind-mounted
docker run against the image parsed out of Dockerfile.lint -- a build
step cannot write fixes back to the worktree -- and its header states
outright that it is a developer convenience, never a gate, and needs a
local daemon.

cmd/vaultik/lintdocker_test.go parses both Dockerfiles and both scripts
and fails if any part of the mechanism is dropped: the digest pin, the
defaultless ARG below `go mod download`, the emptiness guard, the
expansion of the epoch into each check command, the bare per-invocation
epoch assignment in both scripts, cibuild building both files, the
config verification running before the lint, and -- structurally, not
by searching for one retired variable name -- that no script invokes
golangci-lint except through docker. Every one of those losses is
silent: the build still exits 0 and nothing is checked, which is why
they are asserted rather than trusted. The scanner behind the last of
those has its own test, because a structural check that goes blind
passes on every tree, including a broken one.

script/lint takes no arguments now, and says so instead of dropping
them: a build step has no command line to pass linter flags to.

cmd/vaultik/lintdocker_test.go

@@ -0,0 +1,507 @@
+package main_test
+
+import (
+	"os"
+	"path/filepath"
+	"strings"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// This file guards the shape of the lint gate. Every property asserted
+// here is one whose loss is SILENT: the build still exits 0, the gate
+// still looks green, and nothing was linted or tested.
+//
+// The gate is a build step. script/lint builds Dockerfile.lint, which
+// runs golangci-lint as a RUN instruction, so a successful build is a
+// clean lint. BuildKit will happily replay that RUN from cache on an
+// unchanged tree in well under a second, which is why the check layers
+// are keyed on a CHECK_EPOCH build arg that the calling script
+// regenerates per invocation, and why an empty value is a hard error
+// rather than a stable cache key.
+//
+// These are parses rather than invocations. Shelling out to docker from
+// the test suite would nest a build inside `make test`, which itself
+// runs inside a build in CI. The one property a parse cannot establish
+// -- that a real finding actually fails the build -- is verified by
+// hand against a deliberately broken tree, recorded on the pull
+// request.
+
+// The files under guard, relative to the repository root.
+const (
+	lintDockerfile    = "Dockerfile.lint"
+	productDockerfile = "Dockerfile"
+	lintScript        = "script/lint"
+	cibuildScript     = "script/cibuild"
+)
+
+// linterBinary is the linter's command name. Every occurrence of it in
+// executable shell in this repo must be inside a docker invocation; see
+// TestNoHostLintPathRemains.
+const linterBinary = "golangci-lint"
+
+// checkEpochARG is the declaration, with no default value. A default
+// would satisfy the non-empty guard with a constant, and a constant is
+// a stable cache key: the checks would be replayed from cache forever
+// after the first build.
+const checkEpochARG = "ARG CHECK_EPOCH"
+
+// checkEpochGuard is what turns a build that omits --build-arg into a
+// loud failure instead of a quiet green. Failed steps are never cached,
+// so it fires on every such invocation rather than once.
+const checkEpochGuard = `RUN [ -n "$CHECK_EPOCH" ] || exit 1`
+
+// freshEpoch is the epoch computation the calling scripts must use, as
+// a bare assignment on its own line. Inline in an argument, a failing
+// `date` would not abort under `set -eu`; CHECK_EPOCH would become the
+// empty string, and the guard above would be the only thing standing
+// between that and a permanently cached green. `$$` is required because
+// `date +%s` is second-granular and busybox silently drops `%N`, so
+// without the pid two concurrent runs in one second can collide.
+const freshEpoch = `epoch="$(date +%s%N)$$"`
+
+// TestLintDockerfilePinsTheLinterByDigest fails if the lint image stops
+// being pinned. An unpinned tag makes the gate's verdict depend on
+// whatever the registry currently serves under that name.
+func TestLintDockerfilePinsTheLinterByDigest(t *testing.T) {
+	t.Parallel()
+
+	from := ""
+
+	for _, instruction := range instructions(t, lintDockerfile) {
+		if strings.HasPrefix(instruction, "FROM ") {
+			from = instruction
+
+			break
+		}
+	}
+
+	require.NotEmpty(t, from, "%s declares no FROM", lintDockerfile)
+	assert.Contains(t, from, "golangci/golangci-lint",
+		"the lint image must be the golangci-lint image")
+	assert.Contains(t, from, "@sha256:",
+		"the lint image must be pinned by digest, not by tag alone")
+}
+
+// TestLintDockerfileCannotBeCachedGreen pins the whole cache-busting
+// mechanism in the file that lints: the declaration with no default,
+// the non-empty guard, and the value expanded into the lint command
+// itself rather than merely declared.
+func TestLintDockerfileCannotBeCachedGreen(t *testing.T) {
+	t.Parallel()
+
+	found := instructions(t, lintDockerfile)
+
+	argAt := indexOf(found, checkEpochARG)
+	require.GreaterOrEqual(t, argAt, 0,
+		"%s must declare `%s` with no default value",
+		lintDockerfile, checkEpochARG)
+
+	assert.GreaterOrEqual(t, indexOf(found, checkEpochGuard), argAt,
+		"%s must guard against an empty CHECK_EPOCH with `%s`",
+		lintDockerfile, checkEpochGuard)
+
+	assertEpochExpandedInto(t, found[argAt:], "golangci-lint run")
+
+	// Dependency layers must stay above the ARG, or every lint run
+	// re-downloads the module cache and the inner loop becomes
+	// unusable.
+	download := indexOf(found, "RUN go mod download")
+	require.GreaterOrEqual(t, download, 0,
+		"%s must download modules in their own layer", lintDockerfile)
+	assert.Less(t, download, argAt,
+		"`%s` must come after `go mod download` so dependency layers"+
+			" still cache", checkEpochARG)
+}
+
+// TestLintDockerfileVerifiesTheLinterConfig guards the validation of
+// .golangci.yml itself. `golangci-lint run` rejects a config it cannot
+// parse but silently IGNORES an unknown top-level key, so renaming
+// `linters:` to `linterz:` discards `default: all` and every threshold
+// and still exits 0 reporting no issues. `config verify` is what turns
+// that into a failure, and it has to run BEFORE the lint, or the lint
+// spends a minute reporting a verdict from a config already known to be
+// wrong.
+func TestLintDockerfileVerifiesTheLinterConfig(t *testing.T) {
+	t.Parallel()
+
+	found := instructions(t, lintDockerfile)
+	verify := linterBinary + " config verify"
+
+	verifyAt := indexContaining(found, verify)
+	require.GreaterOrEqual(t, verifyAt, 0,
+		"%s must run `%s --config .golangci.yml`: without it a typo'd"+
+			" top-level key in .golangci.yml is silently ignored and the"+
+			" gate passes with only the default linter set", lintDockerfile,
+		verify)
+
+	runAt := indexContaining(found, linterBinary+" run")
+	require.GreaterOrEqual(t, runAt, 0, "%s must lint", lintDockerfile)
+	assert.Less(t, verifyAt, runAt,
+		"%s must verify the config before linting with it", lintDockerfile)
+
+	// Keyed on the epoch like every other check layer, so it executes
+	// per invocation rather than being replayed. A cached validation
+	// validates nothing.
+	assertEpochExpandedInto(t, found, verify)
+}
+
+// TestProductDockerfileCannotBeCachedGreen holds the same line for the
+// checks that remain in the product image build.
+func TestProductDockerfileCannotBeCachedGreen(t *testing.T) {
+	t.Parallel()
+
+	found := instructions(t, productDockerfile)
+
+	argAt := indexOf(found, checkEpochARG)
+	require.GreaterOrEqual(t, argAt, 0,
+		"%s must declare `%s` with no default value",
+		productDockerfile, checkEpochARG)
+
+	assert.GreaterOrEqual(t, indexOf(found, checkEpochGuard), argAt,
+		"%s must guard against an empty CHECK_EPOCH", productDockerfile)
+
+	assertEpochExpandedInto(t, found[argAt:], "make fmt-check")
+	assertEpochExpandedInto(t, found[argAt:], "make test")
+}
+
+// TestProductDockerfileDoesNotLint records the split deliberately: the
+// linter lives in Dockerfile.lint and nowhere else, so there is exactly
+// one digest pinning it. A lint stage reintroduced here would either be
+// docker-in-docker (`make lint` is now `docker build`) or a second,
+// independently bumpable pin.
+func TestProductDockerfileDoesNotLint(t *testing.T) {
+	t.Parallel()
+
+	contents := readRepoFile(t, productDockerfile)
+
+	for _, forbidden := range []string{"golangci", "make lint"} {
+		assert.NotContains(t, instructionText(contents), forbidden,
+			"%s must not lint: the linter is pinned once, in %s",
+			productDockerfile, lintDockerfile)
+	}
+}
+
+// TestLintScriptBuildsTheLintDockerfileWithAFreshEpoch is the other
+// half of the mechanism. The Dockerfile's guard only rejects an EMPTY
+// epoch; a constant non-empty one would satisfy it and still be served
+// from cache forever.
+func TestLintScriptBuildsTheLintDockerfileWithAFreshEpoch(t *testing.T) {
+	t.Parallel()
+
+	script := readRepoFile(t, lintScript)
+
+	assertBareEpochAssignment(t, script, lintScript)
+	assert.Contains(t, script, `--build-arg CHECK_EPOCH="$epoch"`,
+		"%s must pass the fresh epoch to the build", lintScript)
+	assert.Contains(t, script, lintDockerfile,
+		"%s must build %s", lintScript, lintDockerfile)
+}
+
+// TestCibuildBuildsBothDockerfilesWithFreshEpochs guards the CI gate:
+// dropping either build silently removes a whole class of check from
+// CI while leaving it green.
+func TestCibuildBuildsBothDockerfilesWithFreshEpochs(t *testing.T) {
+	t.Parallel()
+
+	script := readRepoFile(t, cibuildScript)
+
+	assertBareEpochAssignment(t, script, cibuildScript)
+	assert.Equal(t, 2, strings.Count(script, freshEpoch),
+		"%s must compute a fresh epoch for each of its two builds",
+		cibuildScript)
+	assert.Equal(t, 2,
+		strings.Count(script, `--build-arg CHECK_EPOCH="$epoch"`),
+		"%s must pass a fresh epoch to both builds", cibuildScript)
+	assert.Contains(t, script, "-f Dockerfile.lint",
+		"%s must build %s", cibuildScript, lintDockerfile)
+}
+
+// TestNoHostLintPathRemains fails if any escape hatch to a host linter
+// comes back. The owner's ruling is that every lint run happens inside
+// a container; a PATH binary that happens to match the pinned version
+// is a different build reached by a different code path, and admitting
+// it is what lets a local pass disagree with CI.
+//
+// This asserts the PROPERTY -- no script invokes the linter except
+// through docker -- rather than the absence of any particular variable
+// name. An earlier version of this test looked only for the literal
+// VAULTIK_LINT_IN_CONTAINER, the name of the hatch that was removed
+// alongside it, so nothing could ever trip it again: a hatch under any
+// other name left it passing. A structural test that passes on a broken
+// tree is worse than no test, because it is what a later reader trusts
+// instead of re-deriving the invariant.
+//
+// script/lint-fix is not exempted. It is the one script that runs the
+// linter as a container rather than as a build step, but it still runs
+// it in one, so the same property holds of it.
+func TestNoHostLintPathRemains(t *testing.T) {
+	t.Parallel()
+
+	root := repoRoot(t)
+
+	entries, err := os.ReadDir(filepath.Join(root, "script"))
+	require.NoError(t, err)
+	require.NotEmpty(t, entries, "no scripts found to scan")
+
+	for _, entry := range entries {
+		if entry.IsDir() {
+			continue
+		}
+
+		name := filepath.Join("script", entry.Name())
+		for _, line := range shellCode(readRepoFile(t, name)) {
+			assertLinterIsContainerised(t, name, line)
+		}
+	}
+}
+
+// assertLinterIsContainerised fails if the line runs the linter without
+// handing it to docker first. Position matters: docker has to come
+// before the binary, or the line is running the host linter and merely
+// mentioning docker afterwards.
+func assertLinterIsContainerised(t *testing.T, name, line string) {
+	t.Helper()
+
+	at := strings.Index(line, linterBinary)
+	if at < 0 {
+		return
+	}
+
+	docker := strings.Index(line, "docker")
+
+	assert.True(t, docker >= 0 && docker < at,
+		"%s runs %s on the host; every lint run happens in a container"+
+			" (line: %s)", name, linterBinary, line)
+}
+
+// TestShellCodeSeesCodeAndNotProse keeps the scanner above honest. It
+// has to ignore comments and here-document bodies, because script/lint
+// and script/bootstrap both NAME golangci-lint in prose -- in comments,
+// and in the error text they print -- precisely to say that the host
+// binary is never used. A scanner that went blind, by over-eager
+// stripping or by failing to join continuation lines, would make
+// TestNoHostLintPathRemains pass on everything.
+func TestShellCodeSeesCodeAndNotProse(t *testing.T) {
+	t.Parallel()
+
+	script := strings.Join([]string{
+		"#!/bin/sh",
+		"# a comment naming golangci-lint",
+		"cat >&2 <<EOF",
+		"prose naming golangci-lint, printed not executed",
+		"EOF",
+		"docker run --rm \\",
+		"    \"$image\" \\",
+		"    golangci-lint run ./...",
+	}, "\n")
+
+	assert.Equal(t,
+		[]string{"cat >&2 <<EOF", `docker run --rm "$image" golangci-lint run ./...`},
+		shellCode(script))
+}
+
+// assertEpochExpandedInto fails unless some instruction runs the named
+// command with the epoch expanded into it. Expansion, not mere
+// declaration: an ARG that no instruction references is not guaranteed
+// to key the layer, and the expansion also puts the value in the build
+// log where a reader can see the layer was keyed fresh.
+func assertEpochExpandedInto(t *testing.T, found []string, command string) {
+	t.Helper()
+
+	for _, instruction := range found {
+		if !strings.HasPrefix(instruction, "RUN ") {
+			continue
+		}
+
+		if strings.Contains(instruction, command) &&
+			strings.Contains(instruction, "${CHECK_EPOCH}") {
+			return
+		}
+	}
+
+	assert.Fail(t, "no epoch-keyed layer runs the command",
+		"`%s` must run in a layer that expands ${CHECK_EPOCH}, or it"+
+			" will be replayed from cache without executing", command)
+}
+
+// assertBareEpochAssignment fails unless the script computes the epoch
+// as a bare assignment on its own line.
+func assertBareEpochAssignment(t *testing.T, script, name string) {
+	t.Helper()
+
+	for line := range strings.SplitSeq(script, "\n") {
+		if strings.TrimSpace(line) == freshEpoch {
+			return
+		}
+	}
+
+	assert.Fail(t, "no bare epoch assignment",
+		"%s must compute `%s` as a bare assignment on its own line, so"+
+			" `set -e` catches a failing date instead of quietly"+
+			" building with an empty epoch", name, freshEpoch)
+}
+
+// instructions returns the Dockerfile's instructions, one per element,
+// with comments and blank lines dropped and continuation lines joined,
+// so a multi-line RUN is one string.
+func instructions(t *testing.T, name string) []string {
+	t.Helper()
+
+	return strings.Split(instructionText(readRepoFile(t, name)), "\n")
+}
+
+// instructionText is instructions' parse, before splitting: it is also
+// what a "must not contain" assertion should look at, so that a word
+// appearing only in a comment is not mistaken for behaviour.
+func instructionText(contents string) string {
+	var (
+		out         []string
+		continued   string
+		isContinued bool
+	)
+
+	for line := range strings.SplitSeq(contents, "\n") {
+		trimmed := strings.TrimSpace(line)
+		if !isContinued && (trimmed == "" || strings.HasPrefix(trimmed, "#")) {
+			continue
+		}
+
+		isContinued = strings.HasSuffix(trimmed, `\`)
+		continued += strings.TrimSuffix(trimmed, `\`)
+
+		if isContinued {
+			continue
+		}
+
+		out = append(out, strings.Join(strings.Fields(continued), " "))
+		continued = ""
+	}
+
+	return strings.Join(out, "\n")
+}
+
+// indexOf returns the position of the first instruction equal to, or
+// beginning with, want; -1 if there is none.
+func indexOf(found []string, want string) int {
+	for i, instruction := range found {
+		if instruction == want || strings.HasPrefix(instruction, want+" ") {
+			return i
+		}
+	}
+
+	return -1
+}
+
+// indexContaining returns the position of the first instruction
+// containing want; -1 if there is none.
+func indexContaining(found []string, want string) int {
+	for i, instruction := range found {
+		if strings.Contains(instruction, want) {
+			return i
+		}
+	}
+
+	return -1
+}
+
+// shellCode returns a POSIX shell script's executable lines: comments
+// dropped, here-document bodies dropped, and backslash continuations
+// joined so a multi-line command is a single string. Whitespace is
+// collapsed, as it is for Dockerfile instructions.
+//
+// Both exclusions are load-bearing rather than tidiness. The scripts
+// name golangci-lint in prose to state that the host binary is never
+// used, and joining continuations is what lets the one legitimate
+// container invocation -- script/lint-fix's `docker run`, whose linter
+// command sits several lines below the word `docker` -- be recognised
+// as containerised.
+func shellCode(contents string) []string {
+	var (
+		out       []string
+		joined    string
+		terminate string
+	)
+
+	for line := range strings.SplitSeq(contents, "\n") {
+		trimmed := strings.TrimSpace(line)
+
+		if terminate != "" {
+			if trimmed == terminate {
+				terminate = ""
+			}
+
+			continue
+		}
+
+		if joined == "" && (trimmed == "" || strings.HasPrefix(trimmed, "#")) {
+			continue
+		}
+
+		joined += strings.TrimSuffix(trimmed, `\`) + " "
+		if strings.HasSuffix(trimmed, `\`) {
+			continue
+		}
+
+		joined = strings.Join(strings.Fields(joined), " ")
+		terminate = heredocTerminator(joined)
+
+		out = append(out, joined)
+		joined = ""
+	}
+
+	return out
+}
+
+// heredocTerminator returns the terminator of the here-document a
+// command opens, or "" if it opens none. Only the first on a line is
+// recognised; nothing in script/ opens two.
+func heredocTerminator(line string) string {
+	_, after, opens := strings.Cut(line, "<<")
+	if !opens {
+		return ""
+	}
+
+	// `<<-` strips leading tabs from the body; the terminator word is
+	// the same either way, and callers compare against trimmed lines.
+	word, _, _ := strings.Cut(strings.TrimPrefix(after, "-"), " ")
+
+	return strings.Trim(word, `'"`)
+}
+
+// readRepoFile reads a file by its path relative to the repository
+// root.
+func readRepoFile(t *testing.T, name string) string {
+	t.Helper()
+
+	//nolint:gosec // G304: the path is a constant relative to this repo
+	contents, err := os.ReadFile(filepath.Join(repoRoot(t), name))
+	require.NoError(t, err)
+
+	return string(contents)
+}
+
+// repoRoot returns the repository root. The test binary runs with its
+// package directory as the working directory, so the root is found by
+// walking up until the module file appears.
+func repoRoot(t *testing.T) string {
+	t.Helper()
+
+	dir, err := os.Getwd()
+	require.NoError(t, err)
+
+	for {
+		_, err = os.Stat(filepath.Join(dir, "go.mod"))
+		if err == nil {
+			return dir
+		}
+
+		parent := filepath.Dir(dir)
+		require.NotEqual(t, dir, parent,
+			"walked to the filesystem root without finding a go.mod")
+
+		dir = parent
+	}
+}

cmd/vaultik/makefile_test.go

@@ -1,8 +1,6 @@
 package main_test
 
 import (
-	"os"
-	"path/filepath"
 	"regexp"
 	"slices"
 	"strings"
@@ -87,27 +85,11 @@ func TestBuildTargetBuildsTheBinary(t *testing.T) {
 }
 
 // readMakefile returns the contents of the repository's Makefile. The
-// test binary runs with its package directory as the working directory,
-// so the root is found by walking up until the Makefile appears.
+// root is located by the shared walk in lintdocker_test.go.
 func readMakefile(t *testing.T) string {
 	t.Helper()
 
-	dir, err := os.Getwd()
-	require.NoError(t, err)
-
-	for {
-		//nolint:gosec // G304: the path is this test's own directory walk
-		contents, err := os.ReadFile(filepath.Join(dir, "Makefile"))
-		if err == nil {
-			return string(contents)
-		}
-
-		parent := filepath.Dir(dir)
-		require.NotEqual(t, dir, parent,
-			"walked to the filesystem root without finding a Makefile")
-
-		dir = parent
-	}
+	return readRepoFile(t, "Makefile")
 }
 
 // phonyTargets returns every name declared phony, across all .PHONY