Rework the lint-guard shell scanner in cmd/vaultik/lintdocker_test.go (second-review findings on the milestone PR) #121

New Issue

2026-09-21T09:19:43+02:00

clawbot commented

2026-09-21 09:19:43 +02:00

The second review of pull request #114 failed on cmd/vaultik/lintdocker_test.go after the lint-in-container unit for issue #113 had already landed on next as d257f8f. The gate mechanism itself held up under execution; what failed is the guard test's own shell scanner, which is weaker than the commit message claims. This issue carries the rework as its own unit on a feature branch off next.

Reviewer findings, verbatim in substance:

heredocTerminator treats any << on a code line as opening a here-document. A << inside a quoted string or an inline comment opens a phantom here-document and the rest of the file is silently dropped from scanning, so an ordinary later edit disarms the guard permanently. An unterminated here-document at end of file is also accepted silently.
assertLinterIsContainerised accepts any line where docker appears before golangci-lint, so docker info >/dev/null; golangci-lint run ./... and docker info || golangci-lint run ./... both pass the guard.

Definition of done

shellCode fails the test loudly when a here-document is still open at end of file, and only a real redirection (<< or <<- followed by a terminator word, not << inside quotes) opens one.
assertLinterIsContainerised splits the joined line on ;, &&, || and | and requires the command that contains the linter to begin with docker. script/lint-fix's continuation-joined docker run ... golangci-lint run ... still passes.
TestShellCodeSeesCodeAndNotProse covers an unterminated here-document and a << that is not one; the mutation coverage in the PR body shows the ; and || evasions above now fail the test and that the real scripts still pass.
The comment on the scanner names the inherent limits of a text scan in one line (a linter name assembled at runtime, a name split across a continuation, scripts in subdirectories, the Makefile) rather than claiming completeness.
Dockerfile.lint:73-76 cites the lll finding; the PR body evidence names a revive finding. Fix the citation.
make check green. No change to Dockerfile.lint, script/lint, script/cibuild beyond the citation in item 5.

Model: fable-5-1

The second review of [pull request #114](https://git.eeqj.de/sneak/vaultik/pulls/114) failed on `cmd/vaultik/lintdocker_test.go` after the lint-in-container unit for [issue #113](https://git.eeqj.de/sneak/vaultik/issues/113) had already landed on `next` as `d257f8f`. The gate mechanism itself held up under execution; what failed is the guard test's own shell scanner, which is weaker than the commit message claims. This issue carries the rework as its own unit on a feature branch off `next`. Reviewer findings, verbatim in substance: 1. `heredocTerminator` treats any `<<` on a code line as opening a here-document. A `<<` inside a quoted string or an inline comment opens a phantom here-document and the rest of the file is silently dropped from scanning, so an ordinary later edit disarms the guard permanently. An unterminated here-document at end of file is also accepted silently. 2. `assertLinterIsContainerised` accepts any line where `docker` appears before `golangci-lint`, so `docker info >/dev/null; golangci-lint run ./...` and `docker info || golangci-lint run ./...` both pass the guard. ## Definition of done 1. `shellCode` fails the test loudly when a here-document is still open at end of file, and only a real redirection (`<<` or `<<-` followed by a terminator word, not `<<` inside quotes) opens one. 2. `assertLinterIsContainerised` splits the joined line on `;`, `&&`, `||` and `|` and requires the command that contains the linter to begin with `docker`. `script/lint-fix`'s continuation-joined `docker run ... golangci-lint run ...` still passes. 3. `TestShellCodeSeesCodeAndNotProse` covers an unterminated here-document and a `<<` that is not one; the mutation coverage in the PR body shows the `;` and `||` evasions above now fail the test and that the real scripts still pass. 4. The comment on the scanner names the inherent limits of a text scan in one line (a linter name assembled at runtime, a name split across a continuation, scripts in subdirectories, the `Makefile`) rather than claiming completeness. 5. `Dockerfile.lint:73-76` cites the `lll` finding; the PR body evidence names a `revive` finding. Fix the citation. 6. `make check` green. No change to `Dockerfile.lint`, `script/lint`, `script/cibuild` beyond the citation in item 5. Model: fable-5-1

clawbot added this to the 1.0.0 milestone 2026-09-21 09:19:43 +02:00

clawbot referenced this issue

2026-09-21 09:20:15 +02:00

Review toward 1.0: identify and file all next steps #119

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sneak/vaultik#121