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..0da3f1f 100644 --- a/prompts/REPO_POLICIES.md +++ b/prompts/REPO_POLICIES.md @@ -691,6 +691,271 @@ 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. $$ is the same idiom CHECK_EPOCH + # uses, for the same reason. + LINT_OUT="$LINT_STATE/run.$$.stdout" + LINT_ERR="$LINT_STATE/run.$$.stderr" + trap 'rm -f "$LINT_OUT" "$LINT_ERR"' EXIT HUP INT 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) + # while findings exit 1, and the other codes in pkg/exitcodes + # carry meanings of their own, so no exit status tells a collision + # apart from a result. + 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. `$$` is + sufficient and is the same idiom the `CHECK_EPOCH` rule uses; `mktemp` + under the state directory is equally fine. The `trap` matters as much as + the paths, or the directory accumulates a pair per run forever. + + 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).