diff --git a/TODO.md b/TODO.md index c598ba2..7965142 100644 --- a/TODO.md +++ b/TODO.md @@ -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 diff --git a/prompts/EXISTING_REPO_CHECKLIST.md b/prompts/EXISTING_REPO_CHECKLIST.md index 83421f9..08e6d0b 100644 --- a/prompts/EXISTING_REPO_CHECKLIST.md +++ b/prompts/EXISTING_REPO_CHECKLIST.md @@ -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 diff --git a/prompts/NEW_REPO_CHECKLIST.md b/prompts/NEW_REPO_CHECKLIST.md index bd6d7f0..dcf8d45 100644 --- a/prompts/NEW_REPO_CHECKLIST.md +++ b/prompts/NEW_REPO_CHECKLIST.md @@ -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 diff --git a/prompts/REPO_POLICIES.md b/prompts/REPO_POLICIES.md index 19304f1..dd870cc 100644 --- a/prompts/REPO_POLICIES.md +++ b/prompts/REPO_POLICIES.md @@ -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 ` 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 -- -`) 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).