Drop the lint-guard shell scanner, keep the Dockerfile.lint checks (closes #121)
check / check (pull_request) Failing after 0s

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
This commit is contained in:
2026-09-21 18:03:15 +00:00
parent 75a10d3a22
commit d4df9701f6
3 changed files with 17 additions and 160 deletions
+7 -151
View File
@@ -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 <<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
@@ -407,70 +327,6 @@ func indexContaining(found []string, want string) int {
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 {