From d4df9701f6b8488cadd1bcd33ba6d1790949d16e Mon Sep 17 00:00:00 2001 From: sneak Date: Mon, 21 Sep 2026 13:04:02 +0000 Subject: [PATCH] Drop the lint-guard shell scanner, keep the Dockerfile.lint checks (closes #121) Four reviews found successive ways a shell script could run the host linter that the hand-written scanner in cmd/vaultik/lintdocker_test.go did not see. A shell parser cannot be complete, and it is the machinery-over-fix the workflow rejects. The tree is already right: script/lint is the only lint entry point and runs golangci-lint only inside the container; keeping it so is a review matter. Remove TestNoHostLintPathRemains and every helper and helper-test that served only it. Keep the plain Dockerfile.lint assertions: pinned image, config verify before run, and the per-run epoch reaching both steps. Correct the TODO.md sentence that claimed a test proves no host lint. Model: opus-4-8 --- Dockerfile.lint | 10 +-- TODO.md | 9 +- cmd/vaultik/lintdocker_test.go | 158 ++------------------------------- 3 files changed, 17 insertions(+), 160 deletions(-) diff --git a/Dockerfile.lint b/Dockerfile.lint index e0413a1..4e39700 100644 --- a/Dockerfile.lint +++ b/Dockerfile.lint @@ -72,11 +72,11 @@ RUN [ -n "$CHECK_EPOCH" ] || exit 1 # running, and exits 0 reporting `0 issues.` on a tree the real config # fails. Demonstrated on this repo at this pin, recorded on # https://git.eeqj.de/sneak/vaultik/pulls/114: with a planted -# over-length line, `script/lint` exits 1 naming the `lll` finding with -# `linters:` and exits 0 with `linterz:`. A set-but-ineffective config -# quietly falling back to defaults is precisely the false-green class -# this gate exists to eliminate, so it must not sit in the gate's own -# configuration. +# over-length line, `script/lint` exits 1 naming the `revive` finding +# with `linters:` and exits 0 with `linterz:`. A set-but-ineffective +# config quietly falling back to defaults is precisely the false-green +# class this gate exists to eliminate, so it must not sit in the gate's +# own configuration. # # `config verify` catches it, and it does so OFFLINE at this pinned # version -- verified, not assumed. Under `docker run --network none` diff --git a/TODO.md b/TODO.md index 5896f00..4ba956a 100644 --- a/TODO.md +++ b/TODO.md @@ -78,10 +78,11 @@ release" is exactly the contradiction into each check command, and a fresh `$(date +%s%N)$$` per invocation computed as a bare assignment. `cmd/vaultik/lintdocker_test.go` parses both Dockerfiles and both scripts and fails if any part of - that is dropped, because every way of losing it is silent. Its - host-lint assertion is structural — no script runs `golangci-lint` - except through `docker` — rather than a search for the one retired - variable name, which nothing could ever reintroduce. + that is dropped, because every way of losing it is silent. No test + asserts that no script runs the host linter: `script/lint` is the one + lint entry point and runs `golangci-lint` only inside the container, + and keeping it that way is a review matter, not something a test + proves. The product `Dockerfile` lost its lint stage rather than gaining a second linter pin: `make lint` is now `docker build`, so the stage diff --git a/cmd/vaultik/lintdocker_test.go b/cmd/vaultik/lintdocker_test.go index 3be8078..2337573 100644 --- a/cmd/vaultik/lintdocker_test.go +++ b/cmd/vaultik/lintdocker_test.go @@ -28,6 +28,11 @@ import ( // -- that a real finding actually fails the build -- is verified by // hand against a deliberately broken tree, recorded on the pull // request. +// +// One property is deliberately NOT tested here: that no script runs the +// linter on the host. script/lint is the only lint entry point, and it +// runs golangci-lint only inside the container; keeping it that way is a +// review matter, not something a test in this file establishes. // The files under guard, relative to the repository root. const ( @@ -37,9 +42,8 @@ const ( 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. +// linterBinary is the linter's command name, used to locate the +// config-verify and lint steps in Dockerfile.lint. const linterBinary = "golangci-lint" // checkEpochARG is the declaration, with no default value. A default @@ -219,90 +223,6 @@ func TestCibuildBuildsBothDockerfilesWithFreshEpochs(t *testing.T) { "%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 <&2 <