sneak/prompts
1
0
Fork 0
Code Issues 18 Pull Requests 4 Actions Packages Projects Releases Wiki Activity

Give golangci-lint per-checkout cache and lock state (closes #30)
All checks were successful
check / check (push) Successful in 7s
Details

Browse Source 
A golangci-lint result on a host running many concurrent workers does not
reliably belong to the tree that asked for it. Two independent mechanisms,
which have repeatedly been mistaken for one:

The result cache is keyed on file content, not location, so two checkouts
of the same commit hold byte-identical files, share cache entries, and one
tree's findings are served for the other under the other tree's path. This
produced a confirmed false green as well as the loud false reds. Moving
workers from shared worktrees to their own clones does not address it —
two clones collide exactly as two worktrees did — and removes only the
foreign-path artefact that made the defect noticeable.

The concurrency lock is $TMPDIR/golangci-lint.lock (pkg/commands/run.go,
acquireFileLock), host-global and independent of GOLANGCI_LINT_CACHE, with
a five-second acquire timeout, so it fails when the host is busiest. A
private cache directory does not isolate it. Setting only the cache closes
the contamination half and leaves runs failing red on a condition that is
not a result at all.

REPO_POLICIES.md now carries the canonical Go script/lint: both variables
scoped into a .lint-cache/ directory inside the checkout, above any
container-versus-host branch so every path reaching the linter gets them;
--allow-serial-runners, which keeps the mutual-exclusion guard and queues
rather than aborting, for the same-checkout overlap TMPDIR scoping cannot
cover, with --allow-parallel-runners rejected because it deletes the
guard; and a bounded retry that treats the lock error as VOID rather than
as findings, exiting 75 on exhaustion so it is neither a pass nor a
failure. Detection is on the stderr stream and never on exit status:
findings go to stdout, so a finding quoting the lock message in source
cannot be retried away, and the exit status is not a stable discriminator
anyway. The interim void rule is recorded with the ../ clause that the
original filter missed, and with its limit stated — it catches
contamination that names foreign files, not contamination that suppresses
findings. Both checklists gained the corresponding items, since a half-fix
that sets only the cache reads as complete.

GOCACHE was measured rather than assumed and does not need isolating: with
the two variables scoped per checkout and GOCACHE shared at the host
default, each checkout reported its own paths.

Verified with the snippet extracted from the committed document and
executed as a consuming repo would adopt it, each control paired against
the pre-fix form: contamination reproduced on the pre-fix script and
absent on the adopted one; a stub linter colliding twice then clearing,
with the retry engaging and succeeding; exhaustion exiting 75 with a VOID
message; a genuine finding whose text quotes the lock message reported as
findings with no retry; and a real held lock failing the pre-fix script
with exit 3 while the adopted script, inheriting the same environment,
completed in one second.
This commit is contained in:
clawbot
2026-08-09 17:35:48 +00:00
parent 3a218497b8
commit 0620416869
4 changed files with 392 additions and 0 deletions
Download Patch File Download Diff File Expand all files Collapse all files

29
TODO.md
View File

@@ -21,6 +21,35 @@ fmt-check, and commit.
# Completed Steps
- 2026-08-09: Made a golangci-lint result belong to the tree that asked for it.
REPO_POLICIES.md now carries the canonical Go `script/lint`, which gives the
linter per-checkout `GOLANGCI_LINT_CACHE` and per-checkout `TMPDIR`. The two
are separate defects and the second is the one that gets dropped: the result
cache is keyed on file content rather than location, so checkouts holding
identical files serve each other's findings under the other's path, while the
concurrency lock is `$TMPDIR/golangci-lint.lock` — host-global, independent of
the cache, and unaffected by isolating it. Moving workers from worktrees to
their own clones does not help either half; it only removes the foreign-path
artefact that made the defect visible. The lock error is retried rather than
surfaced, because it is not a result: it exits non-zero exactly as findings
do, and reporting it as findings sends a correct branch back for rework.
Detection is on the stderr stream and never on exit status, so a finding
quoting the lock message in source cannot be retried away, and exhaustion
exits 75 with a VOID message rather than passing or failing quietly.
`--allow-serial-runners` (which keeps the guard and queues) covers the
same-checkout overlap that `TMPDIR` scoping cannot; `--allow-parallel-runners`
is rejected outright. The stdout and stderr capture files are per invocation
rather than per checkout, because serialising the linter does not serialise
the shell's redirections: two runs in one checkout — the overlap the flag
exists to support — would otherwise truncate and read each other's output,
which is the same defect one layer above where it was fixed. Both checklists
gained the corresponding items, since a half-fix that sets only the cache
reads as complete. `GOCACHE` was measured and does not need isolating.
Verified with the snippet extracted from the committed document and executed
as a consuming repo would adopt it, against paired controls: contamination
reproduced on the pre-fix form and absent on the adopted one, retry engaged,
exhaustion loud, a genuine finding still reported, and a held host lock
failing the pre-fix script while leaving the adopted one untouched.
- 2026-08-09: Kept in-repo agent scratch out of the Docker build context and out
of version control. `.claude/` holds one worktree — an entire additional
checkout of the repo — per in-flight agent, and under `COPY . .` all of it was

7
prompts/EXISTING_REPO_CHECKLIST.md
View File

@@ -100,6 +100,13 @@ with your task.
`script/install-precommit`, shimmed by `make hooks`) runs it
- [ ] README has an **Entrypoints** section documenting the `script/`
entrypoints and linking the standard
- [ ] Go: `script/lint` isolates golangci-lint per checkout —
`GOLANGCI_LINT_CACHE` and `TMPDIR` both exported into `.lint-cache/`
(which is in `.gitignore` and `.dockerignore`), `--allow-serial-runners`
passed, and the lock error retried rather than reported as findings. Copy
the canonical block from `REPO_POLICIES.md`. Setting only the cache is the
common half-fix and leaves `parallel golangci-lint is running` failing
runs red.
- [ ] `make check` does not modify any files in the repo
- [ ] `make test` has a 30-second timeout
- [ ] `make test` runs real tests, not a no-op (at minimum, import/compile

11
prompts/NEW_REPO_CHECKLIST.md
View File

@@ -109,6 +109,17 @@ are thin shims calling them. Model scripts:
- [ ] `script/test` / `make test` — runs real tests, not a no-op (30-second
timeout)
- [ ] `script/lint` / `make lint` — runs linter
- [ ] Go: exports `GOLANGCI_LINT_CACHE` **and** `TMPDIR` into a
`.lint-cache/` directory inside the checkout, above any
container-versus-host branch so every path that reaches the linter
gets them; passes `--allow-serial-runners` (never
`--allow-parallel-runners`); retries on
`parallel golangci-lint is running` detected on **stderr** and exits
75 with a VOID message on exhaustion. Copy the canonical block from
`REPO_POLICIES.md` rather than writing your own: a version that sets
only the cache leaves the false-red half live, and one that detects
the collision by exit status can retry a real finding away.
- [ ] Go: `.lint-cache/` is in both `.gitignore` and `.dockerignore`
- [ ] `script/fmt` / `make fmt` — formats code (writes)
- [ ] `script/fmt-check` / `make fmt-check` — checks formatting (read-only)
- [ ] `script/check` / `make check` — runs `test`, `lint`, `fmt-check`; must not

345
prompts/REPO_POLICIES.md
View File

@@ -691,6 +691,351 @@ style conventions are in separate documents:
missing call site passes every control while the adopted snippet does
nothing.
- **`script/lint` in Go repos must give golangci-lint per-checkout cache and
lock state, and must never report a lock collision as a lint result.**
golangci-lint shares two pieces of state across every process on the host, and
they are separate mechanisms with separate fixes. Isolating one and stopping
leaves the other fully live while reading as a fix. This is independent of the
pinned-install rule above and does not replace it: that one makes the host run
the right linter, this one makes the run's result belong to your own tree.
**Mechanism 1, the result cache — produces false greens as well as false
reds.** golangci-lint keys cached results on file **content, not location**,
so two checkouts of the same commit hold byte-identical files, share cache
entries, and one tree's findings are served for the other — reported at the
_other_ tree's path. Observed across the org: 399 issues attributed to a
`/tmp` worktree that no longer existed, returned from a clean clone that
genuinely lints 0 issues; ten findings against a deleted worktree; findings
reported against `../wt82-lint/...`; and, in the dangerous direction, an
implementer reporting "lint 0 issues" on a branch that was genuinely red.
Note what content-keying implies: **moving agents from worktrees to their
own clones does not help.** Two clones of a repo are byte-identical exactly
as two worktrees were. What own-clones removes is the deleted-worktree path
artefact — the loud, obviously wrong symptom — while leaving the mechanism
live, which makes the defect quieter rather than rarer.
**Mechanism 2, the concurrency lock — and it does not live in the cache
directory.** From `pkg/commands/run.go`, `acquireFileLock()`:
```go
lockFile := filepath.Join(os.TempDir(), "golangci-lint.lock")
```
That is `$TMPDIR/golangci-lint.lock` — host-global, keyed on the temp
directory, entirely independent of `GOLANGCI_LINT_CACHE`. It is an `flock`
retried every second under a **5-second total timeout**, so it fails
precisely when the host is busiest. On failure the run emits
`parallel golangci-lint is running` and analyzes nothing. **A private cache
directory does not prevent this**; that was established by controlled test,
with two concurrent runs under separate cache directories sharing no mounted
path, one of which still collided. Anyone who sets only
`GOLANGCI_LINT_CACHE` has closed the contamination half and left the
false-red half untouched.
The canonical form for a Go repo's `script/lint`:
```sh
#!/bin/sh
# script/lint: run the linter.
set -eu
ROOT="$(cd "$(dirname "$0")/.." && pwd -P)"
# Per-checkout golangci-lint state. Both variables are required and they
# fix different defects; neither is redundant with the other.
#
# GOLANGCI_LINT_CACHE: the result cache is keyed on file content, not
# location, so checkouts holding identical files serve each other's
# findings. A per-REPO cache directory does NOT fix this — every checkout
# of that repo still collides — so the path must be inside the invoking
# checkout.
#
# TMPDIR: golangci-lint flocks $TMPDIR/golangci-lint.lock
# (pkg/commands/run.go, acquireFileLock: filepath.Join(os.TempDir(),
# "golangci-lint.lock")). That lock is host-global and independent of
# GOLANGCI_LINT_CACHE, with a 5s acquire timeout. Scoping TMPDIR into the
# checkout is the only thing here that isolates it. Do not delete this as
# redundant with the cache variable; it is not.
#
# The leading dot in .lint-cache is load-bearing: the go tool skips
# dot-prefixed directories when expanding ./..., so the linter never reads
# its own cache and temp files back as source. Do not rename it.
LINT_STATE="$ROOT/.lint-cache"
GOLANGCI_LINT_CACHE="$LINT_STATE/cache"
TMPDIR="$LINT_STATE/tmp"
export GOLANGCI_LINT_CACHE TMPDIR
mkdir -p "$GOLANGCI_LINT_CACHE" "$TMPDIR"
# Capture files, per INVOCATION and not per checkout. Two runs in the same
# checkout would otherwise redirect into one pair of fixed paths, opened
# O_TRUNC before the linter even starts, and each would print and scan the
# other's output — a run reporting a result that is not its own, which is
# the whole defect this bullet exists to close, one layer up from where it
# was closed. That case is not hypothetical here: two runs in the same
# checkout is exactly what --allow-serial-runners below exists to support,
# and serialising the linter does not serialise the shell's redirections
# or the grep and cat that read them. mktemp rather than $$: two
# containerised runs over one bind-mounted checkout are in separate PID
# namespaces and can both be PID 7, which puts the collision back.
LINT_OUT="$(mktemp "$LINT_STATE/run.out.XXXXXX")"
LINT_ERR="$(mktemp "$LINT_STATE/run.err.XXXXXX")"
# Print whatever the linter had written before the interruption, then exit
# 128+signal. The exit is what makes this handler TERMINATING, and that is
# the point: a signal-trap handler that returns RESUMES the script. With
# the capture files already deleted, execution would fall into the grep
# below against a missing file, take the not-a-collision branch, and report
# the FINDINGS exit status with empty output for a run that was killed —
# after deleting the findings it was about to print. That is why cleanup
# is on EXIT only. Killing a run is not hypothetical here: it is the stated
# mitigation for the unbounded wait --allow-serial-runners can produce, and
# it is what Ctrl-C on a make check does.
#
# All three writes are best-effort, and the `|| :` on each is load-bearing
# rather than defensive habit. Under set -e a failed write aborts the
# function BEFORE exit "$1", and the shell then exits 1 — the findings
# status, on a run that analysed nothing. SIGHUP is precisely the case
# where writing fails: once the controlling terminal is gone the writes
# return EIO. Guarding only the two cats is not enough, because with the
# capture files empty the cats write nothing and succeed, and the echo is
# what fails.
lint_interrupted() {
if [ -f "$LINT_ERR" ]; then cat "$LINT_ERR" >&2 || :; fi
if [ -f "$LINT_OUT" ]; then cat "$LINT_OUT" || :; fi
echo "lint: interrupted by a signal, so nothing was completed." \
"This is NOT a lint result." >&2 || :
exit "$1"
}
# The EXIT trap still fires on the way out of a terminating handler, so
# cleanup happens exactly once on every path. `|| :` because a failing rm
# — an unwritable state directory is enough — would otherwise change the
# exit status of an otherwise clean run under set -e.
trap 'rm -f "$LINT_OUT" "$LINT_ERR" || :' EXIT
trap 'lint_interrupted 129' HUP
trap 'lint_interrupted 130' INT
trap 'lint_interrupted 143' TERM
# Backstop for a caller that reached the linter without the environment
# above. With it set, this should never fire.
LINT_MAX_ATTEMPTS=5
# EX_TEMPFAIL. Distinct from 1 (findings) and 3 (linter error) so a void
# run is never counted as either.
LINT_VOID_EXIT=75
golangci_lint_run() {
attempt=1
delay=2
while :; do
rc=0
# --allow-serial-runners KEEPS the mutual-exclusion guard and makes
# an overlapping run queue on the lock instead of aborting after
# 5s. It is NOT --allow-parallel-runners, which removes the guard
# entirely; never use that one. This is what covers two runs inside
# the SAME checkout, which TMPDIR scoping cannot — script/precommit
# overlapping a make check is the realistic trigger.
golangci-lint run --allow-serial-runners "$@" \
>"$LINT_OUT" 2>"$LINT_ERR" || rc=$?
# Detect the lock collision on the STDERR STREAM, never on the exit
# status. Findings are written to stdout and golangci-lint reports
# this failure only on stderr, so a finding that quotes the string
# from source cannot be mistaken for a collision and retried away —
# that direction would be a false green. The exit status is not a
# usable discriminator: the collision exits 3 (exitcodes.Failure),
# which does separate it from findings at 1, but run.go returns it
# as a plain error that Execute maps to Failure like every other
# error at that level, so 3 cannot separate a collision from a
# genuine linter failure — an unknown linter name, an unknown flag,
# malformed config YAML all exit 3 too. (An unparseable Go source
# file does not: that is reported as typecheck issues and exits 1.)
# Retrying on 3 would retry real failures into a void.
if ! grep -q 'parallel golangci-lint is running' "$LINT_ERR"; then
cat "$LINT_ERR" >&2
cat "$LINT_OUT"
return "$rc"
fi
if [ "$attempt" -ge "$LINT_MAX_ATTEMPTS" ]; then
cat "$LINT_ERR" >&2
echo "lint: VOID after $LINT_MAX_ATTEMPTS attempts:" \
"golangci-lint never acquired its lock, so nothing was" \
"analyzed. This is NOT a lint result and no verdict may" \
"be recorded from it. Re-run it." >&2
return "$LINT_VOID_EXIT"
fi
echo "lint: lock held by another golangci-lint; attempt" \
"$attempt of $LINT_MAX_ATTEMPTS, retrying in ${delay}s" >&2
sleep "$delay"
attempt=$((attempt + 1))
delay=$((delay * 2))
done
}
main() {
cd "$ROOT"
golangci_lint_run ./...
}
main "$@"
```
Load-bearing properties, each guarding a mode that otherwise reports a
verdict it did not earn:
- **Both variables, per checkout.** Cache alone leaves the false reds;
`TMPDIR` alone leaves the contamination that produced a confirmed false
green. A per-repo path for either is not isolation on a host where every
worker holds its own copy of the same repo.
- **Set them on every path that reaches the linter.** The export block goes
_above_ any container-versus-host branch, and a native escape hatch must
call `golangci_lint_run` rather than `exec golangci-lint` directly. One
repo in the org had exactly one such path with no cache environment at
all, inheriting the fleet-wide default, so the fix on the other path was
worth nothing there.
- **The lock collision is not a result, and must never be reported as one.**
Retry it, and on exhaustion exit a status that is neither the findings
status nor success, with a message that says VOID. Swallowing it into a
success is the worst available outcome; reporting it as findings sends a
correct branch back for rework against findings that do not exist.
- **Detect the collision by the message on stderr, not by exit status, and
never retry a genuine finding.** Distinguishing "exited non-zero because
of the lock" from "exited non-zero because of findings" is the whole crux,
and getting it wrong in the direction of treating findings as a lock error
retries a real failure into a void — or, if a later implementation decided
to treat exhaustion as success, into a green.
- **`--allow-serial-runners`, never `--allow-parallel-runners`.** The first
keeps the guard and queues; the second deletes it and lets two runs
corrupt shared state. With the flag set, an overlapping run inside the
same checkout waits rather than failing, which is what is actually wanted.
The honest cost is that it waits without bound, so a stale process holding
the lock hangs the run instead of failing it; the contending set is
bounded to the same checkout, and an eventual result is preferable to a
fabricated one.
- **Capture stdout and stderr to per-INVOCATION paths, and clean them up.**
Two fixed paths under the checkout are one pair for every run in it, and
the redirections truncate them before the linter starts, so two
overlapping runs print and scan each other's output.
`--allow-serial-runners` does not prevent this — it serialises the linter,
not the shell — and the overlap it exists to support is precisely
`script/precommit` against a `make check` in one checkout. The observed
shapes are a run printing the other's `0 issues.` while its own linter
found something, and a lock error erased before `grep` reads it, so the
retry never fires and the void run returns as a result. Both are a run
reporting a result that is not its own, which is this bullet's entire
subject reintroduced one layer above where it was fixed. Use `mktemp`
under the state directory rather than `$$`: two containerised runs over
one bind-mounted checkout sit in separate PID namespaces and can hold the
same low PID, which puts the collision back on exactly the fleet's
arrangement. `$$` is acceptable only where `mktemp` is unavailable and
that arrangement is ruled out.
- **Clean up on `EXIT` only, and give each signal a TERMINATING handler.** A
signal-trap handler that does not exit **resumes** the script: with the
capture files already deleted, the run falls into the `grep` against a
missing file, takes the not-a-collision branch, and reports the
**findings** exit status with empty output for a run that was killed —
having deleted the findings it was about to print. Measured: `TERM`, `INT`
and `HUP` all returning 1 with empty stdout, against 143, 130 and 129 for
the same block without the handler. Exit `128+signal` instead, and let the
`EXIT` trap do the cleanup on the way out. The handler also makes a
**best-effort** attempt to print what the linter had already written —
best-effort because under `SIGHUP` the terminal is typically gone and
every write returns `EIO`, in which case nothing is printed and only the
status carries the message. Each of those writes needs its own `|| :`:
under `set -e` a failed write aborts the handler before it reaches `exit`,
and the shell then exits 1, which is the findings status on a run that
analysed nothing. Guarding only the `cat`s is not enough — with the
capture files empty they write nothing and succeed, and the `echo` is what
fails. Put `|| :` on the `rm` in the `EXIT` trap for the same reason: an
unwritable state directory makes it fail, and a failing `EXIT` trap under
`set -e` turns an otherwise clean run into exit 1, measured in both `dash`
and `bash`.
- **A signal must reach the LINTER, not just the wrapper.** POSIX defers a
trap until the running foreground command completes, so
`kill -TERM <wrapper pid>` does nothing at all while `golangci-lint` is
running — measured still alive three seconds later, where the same block
without a handler dies immediately at 143. Ctrl-C is unaffected because
the terminal signals the whole process group. This matters exactly where
the handler is supposed to help: the unbounded `--allow-serial-runners`
wait, where the process holding things up is the linter itself. Kill the
group (`kill -- -<pgid>`) or use Ctrl-C.
- **Known, accepted gap:** a signal arriving between the `mktemp` calls and
the `trap ... EXIT` line leaves the two capture files behind. It is
closable, and cheaply — initialise both variables to the empty string and
move all four `trap` lines above the `mktemp` calls, with nothing
rewritten afterwards. It is accepted anyway because of what the gap costs,
not because of what closing it costs: two stray files in a gitignored
directory, never an incorrect result. Reconsider it on that trade-off if
the balance ever changes.
Adopting repos must add `.lint-cache/` to both `.gitignore` and
`.dockerignore`. The second matters as much as the first: the directory
reaches tens of megabytes, and without the exclusion it enters the build
context and invalidates `COPY . .` for reasons unrelated to the repo's
content.
**`GOCACHE` does not need isolating, and this was measured rather than
assumed.** With `GOLANGCI_LINT_CACHE` and `TMPDIR` per checkout and
`GOCACHE` left at the host default and shared, two checkouts of identical
content each reported their own paths and neither reported the other's. The
Go build cache is content-addressed and its entries are compiled artifacts
rather than diagnostics carrying a foreign tree's paths, and it has no
equivalent global lock — the whole fleet compiles concurrently against one
`GOCACHE` all day without a contention error. Isolating it would cost a full
cold compile per checkout for no measured benefit. The earlier hypothesis
that Go build-cache contention might explain the lock error is superseded:
the lock is located in source at `$TMPDIR/golangci-lint.lock`.
**Verifying a change to this logic requires a negative control, and the
control must be built out of checkouts with identical content.** Create two
checkouts of the same tree containing a deliberate lint finding, run the
linter in the second so it populates the cache, then run it in the first and
confirm the finding is reported at the first checkout's own path and never
at the second's. Run the same control against the unisolated form and
confirm the contamination appears there — a control that passes against the
broken implementation proves nothing. Note specifically that a control built
from checkouts whose **content differs** passes against the unisolated form
too, because differing content does not collide in a content-keyed cache, so
it is not a test of anything. For the lock half, hold
`$TMPDIR/golangci-lint.lock` with `flock` and confirm the run queues rather
than aborting, that a caller never sees the collision as findings, and that
exhaustion fails loudly and distinguishably.
**Run those controls against the block as a consuming repo would adopt it**
— pasted into a `script/lint`-shaped file that is then executed, not sourced
with the functions driven by hand. The same warning as for the bootstrap
block above, for the same reason.
- **Interim rule for reading a golangci-lint result on a shared host, until
every repo has adopted the isolation above.** A lint run is **VOID** unless
both hold:
- the output contains no `parallel golangci-lint is running`, and
- no reported file path begins with `../`, and none is an absolute path
outside the tree the run was launched from.
Do not record a verdict from a void run, and do not "fix" findings in files
the change does not touch — chasing phantom findings across untouched files
puts unrelated edits into a reviewed diff, which is more expensive than the
wasted rework.
The `../` clause is the one that actually bites, and it is why a filter
keyed on `/tmp` or on absolute prefixes is not enough: golangci-lint reports
paths relative to its own resolved root rather than yours, and three of the
org's reported sightings had relative paths and would have passed such a
filter. Both clauses are needed and neither alone is sufficient — one
reproduction exited non-zero with the lock error and no foreign paths at
all, and another reported 34 well-formed findings, every one of them against
another checkout.
**State the limit of these tests rather than treating them as a guarantee.**
They catch contamination that **names** foreign files. They cannot catch
contamination that **suppresses** findings through a poisoned entry for
colliding content, which has no wall-clock tell either — **no evidence of
that mode has been observed, and nobody should go chasing it**; the point is
the reach of the tests, not a claim that the mode exists. They are a filter
for the loud mode, not a proof of soundness — which is the whole argument
for fixing this in the tooling instead of documenting a discipline that
depends on every agent remembering to apply it.
- When pinning images or packages by hash, add a comment above the reference
with the version and date (YYYY-MM-DD).