script/lint uses the host golangci-lint and a shared cache, so lint results can be wrong in either direction #106

New Issue

clawbot · 2026-08-09T08:09:02+02:00

clawbot commented

2026-08-09 08:09:02 +02:00

@sneak — assigning to you because the durable fix belongs in the canonical script/lint, which is a shared Scripts to Rule Them All file, not a webhooker file. Same situation as #98.

What I observed, in this repo, today

script/lint runs the host golangci-lint binary:

golangci-lint run --config .golangci.yml ./...

The host binary uses one cache (~/.cache/golangci-lint) and one lock, shared by every concurrent session on this machine — and there are many, all linting from throwaway git worktrees. Two failure modes, both reproduced here within five minutes, both launched from the clean shared clone at /srv/code/webhooker on main @ 4f5ecb1:

Lock collision. Error: parallel golangci-lint is running / make: *** [Makefile:16: lint] Error 3. This is not a lint result at all, but it exits non-zero, so anything treating non-zero as "lint failed" will wrongly fail a good change.
Cross-worktree contamination. The next attempt returned a plausible-looking 34 issues (revive 11, nolintlint 16, gosec 2, gochecknoglobals 4, gochecknoinits 1) — every one attributed to files under ../wt82-lint/internal/..., a worktree that had already been deleted. A third attempt from the same directory returned 0 issues.

The deleted-worktree detail matters: the cache retains entries keyed to paths that no longer exist, so cleaning up worktrees does not prevent this. Only cache isolation does.

Why this is worse than it sounds

A lint gate here can currently produce an unearned red (case 1), another codebase's findings presented as yours (case 2), or — combined with the separate script/cibuild Docker-cache issue — an unearned green. An agent or developer acting on case 2 would "fix" phantom findings in files they never touched.

Detection note for anyone writing a guard: the contaminated paths are relative (../wt82-lint/...), not absolute /tmp/.... A filter keyed on absolute or /tmp prefixes passes the bad run as valid. The reliable test is: void the run if any reported path begins with ../, or with an absolute path outside the worktree it was launched from.

Corrected fact, for the record

While verifying the above I disproved a claim that had been circulating in this repo's review notes (including my own briefs) — that the host linter is v2.10.1 and reports a pre-existing gosec G704 in internal/delivery/client_ssrf_test.go:

golangci-lint has version 2.12.2 — identical to the Dockerfile pin. There is no host/CI version skew on this machine now.
A valid run on clean main reports 0 issues. The G704 does not reproduce.

The host was most likely genuinely older earlier and has since been upgraded, so both observations were true at different times — but the stale version is the one that ended up in review briefs, where it was being used to discount a lint finding. No merge verdict depended on it: Gitea CI runs the pinned v2.12.2 lint stage in-container with its own cache, is immune to all of this, and was green on every branch marked merge-ready.

Options

Per-worktree cache in canonical script/lint — export a GOLANGCI_LINT_CACHE under the repo root before invoking. Smallest change, fixes both modes, keeps the host binary. This is what has been proposed upstream.
Run the pinned Docker image from script/lint — the same digest the Dockerfile already uses, giving each run a container-local cache and eliminating host/CI version drift permanently. Stronger guarantee; makes make lint require Docker and slower.
Do nothing local, rely on CI. Already effectively the case for merge decisions, but it leaves every local make lint and every pre-commit hook unreliable.

Recommendation

Option 1 in the canonical script/lint, propagated to all repos. It is a two-line change, fixes the lock collisions and the contamination together, and does not make the common path slower or Docker-dependent. Option 2 is tempting for the version-drift guarantee, but script/precommit calls script/lint, and making every commit shell out to Docker is a real cost.

Definition of done

Canonical script/lint isolates the linter cache per repo or per worktree.
This repo's script/lint refreshed from the canonical copy.
Concurrent make lint invocations from two worktrees produce neither parallel golangci-lint is running nor findings attributed to the other worktree.
make lint on a clean main still reports 0 issues. and CI stays green with the v2.12.2 pin unchanged.

@sneak — assigning to you because the durable fix belongs in the canonical `script/lint`, which is a shared Scripts to Rule Them All file, not a webhooker file. Same situation as #98. ## What I observed, in this repo, today `script/lint` runs the **host** `golangci-lint` binary: ```sh golangci-lint run --config .golangci.yml ./... ``` The host binary uses one cache (`~/.cache/golangci-lint`) and one lock, shared by every concurrent session on this machine — and there are many, all linting from throwaway git worktrees. Two failure modes, both reproduced here within five minutes, both launched from the clean shared clone at `/srv/code/webhooker` on `main` @ `4f5ecb1`: 1. **Lock collision.** `Error: parallel golangci-lint is running` / `make: *** [Makefile:16: lint] Error 3`. This is not a lint result at all, but it exits non-zero, so anything treating non-zero as "lint failed" will wrongly fail a good change. 2. **Cross-worktree contamination.** The next attempt returned a plausible-looking **34 issues** (`revive` 11, `nolintlint` 16, `gosec` 2, `gochecknoglobals` 4, `gochecknoinits` 1) — every one attributed to files under **`../wt82-lint/internal/...`**, a worktree that had *already been deleted*. A third attempt from the same directory returned `0 issues.` The deleted-worktree detail matters: the cache retains entries keyed to paths that no longer exist, so cleaning up worktrees does not prevent this. Only cache isolation does. ## Why this is worse than it sounds A lint gate here can currently produce an unearned red (case 1), another codebase's findings presented as yours (case 2), or — combined with the separate `script/cibuild` Docker-cache issue — an unearned green. An agent or developer acting on case 2 would "fix" phantom findings in files they never touched. **Detection note for anyone writing a guard:** the contaminated paths are **relative** (`../wt82-lint/...`), not absolute `/tmp/...`. A filter keyed on absolute or `/tmp` prefixes passes the bad run as valid. The reliable test is: void the run if any reported path begins with `../`, or with an absolute path outside the worktree it was launched from. ## Corrected fact, for the record While verifying the above I disproved a claim that had been circulating in this repo's review notes (including my own briefs) — that the host linter is v2.10.1 and reports a pre-existing `gosec` G704 in `internal/delivery/client_ssrf_test.go`: - `golangci-lint has version 2.12.2` — **identical to the Dockerfile pin**. There is no host/CI version skew on this machine now. - A valid run on clean `main` reports **`0 issues.`** The G704 does not reproduce. The host was most likely genuinely older earlier and has since been upgraded, so both observations were true at different times — but the stale version is the one that ended up in review briefs, where it was being used to *discount* a lint finding. No merge verdict depended on it: Gitea CI runs the pinned v2.12.2 lint stage in-container with its own cache, is immune to all of this, and was green on every branch marked merge-ready. ## Options 1. **Per-worktree cache in canonical `script/lint`** — export a `GOLANGCI_LINT_CACHE` under the repo root before invoking. Smallest change, fixes both modes, keeps the host binary. This is what has been proposed upstream. 2. **Run the pinned Docker image from `script/lint`** — the same digest the Dockerfile already uses, giving each run a container-local cache and eliminating host/CI version drift permanently. Stronger guarantee; makes `make lint` require Docker and slower. 3. **Do nothing local, rely on CI.** Already effectively the case for merge decisions, but it leaves every local `make lint` and every pre-commit hook unreliable. ## Recommendation Option 1 in the canonical `script/lint`, propagated to all repos. It is a two-line change, fixes the lock collisions and the contamination together, and does not make the common path slower or Docker-dependent. Option 2 is tempting for the version-drift guarantee, but `script/precommit` calls `script/lint`, and making every commit shell out to Docker is a real cost. ## Definition of done - Canonical `script/lint` isolates the linter cache per repo or per worktree. - This repo's `script/lint` refreshed from the canonical copy. - Concurrent `make lint` invocations from two worktrees produce neither `parallel golangci-lint is running` nor findings attributed to the other worktree. - `make lint` on a clean `main` still reports `0 issues.` and CI stays green with the v2.12.2 pin unchanged.

sneak was assigned by clawbot

2026-08-09 08:09:03 +02:00

clawbot referenced this issue

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

Add inactivity-based session timeout (closes #66) #105

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sneak/webhooker#106