sneak/vaultik
1
0
Fork 0
Code Issues 19 Pull Requests 1 Actions Packages Projects Releases Wiki Activity

script/lint shares one golangci-lint cache across worktrees, so one tree's findings are served for another #99

New Issue
Closed
opened 2026-08-09 12:12:09 +02:00 by clawbot · 2 comments
clawbot commented 2026-08-09 12:12:09 +02:00
Collaborator
Copy Link

Found while verifying PR #98. A new variant of #88, with a different
signature: no parallel golangci-lint is running message appears, so
the existing VOID heuristic does not catch it.

What happened

make lint from a worktree at /tmp/impl-93 reported 231 issues.
Every one of the 232 finding lines cited a path under ../impl-91/ — a
different worktree on the same host, belonging to another session. Not
one finding referred to a file in the tree being linted. Example lines:

../impl-91/internal/vaultik/restore_locality_test.go:1:9: package should be `vaultik_test` instead of `vaultik` (testpackage)
../impl-91/internal/database/repository_edge_cases_test.go:412:6: TestLargeDatasets's subtests should call t.Parallel (tparallel)

Re-running with an isolated XDG_CACHE_HOME and nothing else changed:
0 issues, 0 foreign paths.

Mechanism

script/lint mounts only its own $ROOT at /src, so the container
cannot see the other worktree's files. The leak is the cache, not the
mount:

cache_root() {
    echo "${XDG_CACHE_HOME:-${HOME:-/tmp}/.cache}/vaultik-lint"
}
...
    --env GOLANGCI_LINT_CACHE=/cache/golangci-lint \
    --volume "$cache:/cache" \

That path is shared by every worktree of this repo on the host. Two
worktrees of the same repo have identical Go file contents, so their
cache keys collide, and golangci-lint replays the stored analysis
results — including the paths recorded when they were produced, which is
why the other worktree's ../impl-91/ paths surface verbatim.

Why this matters more than it looks

The failure is symmetric, and only one direction is loud:

  • A clean tree can be failed by a dirty sibling. Noisy, gets
    investigated.
  • A dirty tree can be passed by a clean sibling. Silent, and a green
    from a lint run that analysed someone else's code is a sixth way for a
    gate in this repo to report a green it did not earn — alongside #78,
    #80, #85, #88, #93.

The absence of a parallel golangci-lint is running message means the
#88 VOID rule as currently stated does not reject these runs. The
signature to add is: any finding citing a path outside the tree being
linted, and any run whose findings are entirely foreign, is void.

Not affected

The containerised lint inside script/cibuild is structurally immune —
the lint stage runs in the image with its own cache and no shared volume
— and passed on the same tree.

Possible directions

Not prescribing one; script/lint is currently treated as off-limits
under #78/#80/#88, so this needs a deliberate decision.

  1. Key the cache directory on the worktree path (e.g. a hash of $ROOT)
    so worktrees cannot collide. Keeps the warm-cache speed that the
    comment in script/lint says the shared cache exists for, at the
    cost of one cache per worktree.
  2. Take a lock around the cache so runs serialise.
  3. Accept the risk but make it detectable — have script/lint fail
    loudly if any reported path lies outside $ROOT. Cheap, and it turns
    the silent-pass direction into a hard error.

Option 3 is worth doing regardless of which of the others is chosen,
since it converts an unearned green into a failure.

Reproducing

Needs two worktrees of this repo on one host with differing lint
cleanliness, then make lint from the clean one.

Found while verifying PR #98. A new variant of #88, with a different signature: no `parallel golangci-lint is running` message appears, so the existing VOID heuristic does not catch it. ## What happened `make lint` from a worktree at `/tmp/impl-93` reported **231 issues**. Every one of the 232 finding lines cited a path under `../impl-91/` — a different worktree on the same host, belonging to another session. Not one finding referred to a file in the tree being linted. Example lines: ``` ../impl-91/internal/vaultik/restore_locality_test.go:1:9: package should be `vaultik_test` instead of `vaultik` (testpackage) ../impl-91/internal/database/repository_edge_cases_test.go:412:6: TestLargeDatasets's subtests should call t.Parallel (tparallel) ``` Re-running with an isolated `XDG_CACHE_HOME` and nothing else changed: **0 issues, 0 foreign paths.** ## Mechanism `script/lint` mounts only its own `$ROOT` at `/src`, so the container cannot see the other worktree's files. The leak is the cache, not the mount: ```sh cache_root() { echo "${XDG_CACHE_HOME:-${HOME:-/tmp}/.cache}/vaultik-lint" } ... --env GOLANGCI_LINT_CACHE=/cache/golangci-lint \ --volume "$cache:/cache" \ ``` That path is shared by every worktree of this repo on the host. Two worktrees of the same repo have identical Go file contents, so their cache keys collide, and golangci-lint replays the stored analysis results — including the paths recorded when they were produced, which is why the other worktree's `../impl-91/` paths surface verbatim. ## Why this matters more than it looks The failure is symmetric, and only one direction is loud: * A clean tree can be **failed** by a dirty sibling. Noisy, gets investigated. * A dirty tree can be **passed** by a clean sibling. Silent, and a green from a lint run that analysed someone else's code is a sixth way for a gate in this repo to report a green it did not earn — alongside #78, #80, #85, #88, #93. The absence of a `parallel golangci-lint is running` message means the #88 VOID rule as currently stated does not reject these runs. The signature to add is: **any finding citing a path outside the tree being linted, and any run whose findings are entirely foreign, is void.** ## Not affected The containerised lint inside `script/cibuild` is structurally immune — the lint stage runs in the image with its own cache and no shared volume — and passed on the same tree. ## Possible directions Not prescribing one; `script/lint` is currently treated as off-limits under #78/#80/#88, so this needs a deliberate decision. 1. Key the cache directory on the worktree path (e.g. a hash of `$ROOT`) so worktrees cannot collide. Keeps the warm-cache speed that the comment in `script/lint` says the shared cache exists for, at the cost of one cache per worktree. 2. Take a lock around the cache so runs serialise. 3. Accept the risk but make it detectable — have `script/lint` fail loudly if any reported path lies outside `$ROOT`. Cheap, and it turns the silent-pass direction into a hard error. Option 3 is worth doing regardless of which of the others is chosen, since it converts an unearned green into a failure. ## Reproducing Needs two worktrees of this repo on one host with differing lint cleanliness, then `make lint` from the clean one.
clawbot added this to the 1.0.0 milestone 2026-08-09 12:14:24 +02:00
clawbot commented 2026-08-09 16:32:15 +02:00
Author
Collaborator
Copy Link

Implementation plan (this issue is being fixed together with #80 in one
PR, branch fix-lint-isolation, since both are the same defect in
script/lint: the native path is gated on version equality rather than
execution context, and cache isolation is part of that context).

For this issue:

  1. Per-worktree cache key. cache_root() becomes
    ${XDG_CACHE_HOME:-~/.cache}/vaultik-lint/<basename>-<hash of $ROOT>, so two worktrees can no longer collide. The cache stays
    persistent per worktree, so the warm-run speed the shared cache
    existed for is kept.

  2. Foreign-path guard (the item that matters most). script/lint
    stops execing the linter and instead captures its output (still
    streamed live via tee), then rejects the run if any finding cites a
    path that is not inside the tree being linted: absolute paths outside
    the root, any path with a .. component, and any relative path that
    does not exist under $ROOT. That last one also catches a foreign
    path that happens to lack ... A run that trips this exits non-zero
    with a diagnostic naming the cache directory and how to clear it —
    which converts the silent-pass direction into a hard error regardless
    of whether (1) holds.

  3. parallel golangci-lint is running becomes a retry, not a
    verdict.     Per the empirical finding in #88 a private cache does not
    remove lock contention. That message is not a lint result, and
    surfacing it as a non-zero exit is indistinguishable to a caller from
    real findings. It will be retried a bounded number of times with a
    backoff, and if the retries are exhausted the script fails with a
    message that says explicitly that this is not a lint verdict.

  4. Bounded cache growth. Per-worktree cache directories carry a
    marker file recording the worktree path; on each run, sibling cache
    directories whose recorded worktree no longer exists are removed.
    Since worktrees under /tmp are the ones that proliferate, this
    collects them as they disappear. The location and how to clear it by
    hand get documented.

Verification will be the actual reproduction, not inspection: two
concurrent make lint runs from two worktrees of this repo with
differing lint cleanliness, confirming each reports only its own
findings and neither dies on the parallel-run error; plus a constructed
run whose findings cite an outside path, confirming a non-zero exit.
.golangci.yml is not touched (sha256 verified before push), nor is the
lint-stage FROM line or the CHECK_EPOCH structure.

Implementation plan (this issue is being fixed together with #80 in one PR, branch `fix-lint-isolation`, since both are the same defect in `script/lint`: the native path is gated on version equality rather than execution context, and cache isolation is part of that context). For this issue: 1. **Per-worktree cache key.** `cache_root()` becomes `${XDG_CACHE_HOME:-~/.cache}/vaultik-lint/<basename>-<hash of $ROOT>`, so two worktrees can no longer collide. The cache stays persistent per worktree, so the warm-run speed the shared cache existed for is kept. 2. **Foreign-path guard (the item that matters most).** `script/lint` stops `exec`ing the linter and instead captures its output (still streamed live via `tee`), then rejects the run if any finding cites a path that is not inside the tree being linted: absolute paths outside the root, any path with a `..` component, and any relative path that does not exist under `$ROOT`. That last one also catches a foreign path that happens to lack `..`. A run that trips this exits non-zero with a diagnostic naming the cache directory and how to clear it — which converts the silent-pass direction into a hard error regardless of whether (1) holds. 3. **`parallel golangci-lint is running` becomes a retry, not a verdict.** Per the empirical finding in #88 a private cache does not remove lock contention. That message is not a lint result, and surfacing it as a non-zero exit is indistinguishable to a caller from real findings. It will be retried a bounded number of times with a backoff, and if the retries are exhausted the script fails with a message that says explicitly that this is not a lint verdict. 4. **Bounded cache growth.** Per-worktree cache directories carry a marker file recording the worktree path; on each run, sibling cache directories whose recorded worktree no longer exists are removed. Since worktrees under `/tmp` are the ones that proliferate, this collects them as they disappear. The location and how to clear it by hand get documented. Verification will be the actual reproduction, not inspection: two concurrent `make lint` runs from two worktrees of this repo with differing lint cleanliness, confirming each reports only its own findings and neither dies on the parallel-run error; plus a constructed run whose findings cite an outside path, confirming a non-zero exit. `.golangci.yml` is not touched (sha256 verified before push), nor is the lint-stage `FROM` line or the `CHECK_EPOCH` structure.
clawbot commented 2026-08-09 16:48:54 +02:00
Author
Collaborator
Copy Link

Implemented in PR #102 (branch fix-lint-isolation, one commit,
together with #80 since they are the same defect).

What was built

  1. The cache is now keyed per worktree:
    ${XDG_CACHE_HOME:-~/.cache}/vaultik-lint/<slug>-<12 hex of a digest of $ROOT>. Still persistent, so the warm-run speed the shared cache
    existed for is kept; a collision between two checkouts is no longer
    possible.
  2. New script/lint-audit, run on every lint, rejects output citing any
    file that is not in the tree being linted — absolute paths outside
    the root, .. components, and relative paths that do not exist here.
    It runs on clean output too, since the unearned-pass direction is
    the silent one. It deliberately never certifies that a run passed: it
    does not look at whether there were findings, so it cannot become a
    second gate that reports a green. It is a separate script rather than
    an inline function so it can be exercised directly against a saved
    capture, which is how it was verified.
  3. parallel golangci-lint is running is now a bounded retry (6
    attempts, 15s apart), not a verdict. Exhausting the retries fails
    with a message stating the tree was never analysed. Confirmed the
    #88 reading that this is orthogonal to cache privacy.
    --allow-parallel-runners was rejected as the alternative: it
    disables the lock that protects a cache shared by two runs of the
    same worktree, trading a delay for corruption.
  4. Each cache records the worktree it belongs to and is collected on the
    next run once that worktree is gone, so throwaway worktrees do not
    accumulate caches. The whole tree is under XDG_CACHE_HOME and
    disposable; the removal command appears in the script comment and in
    the rejection message.

How it was verified (reproduction, not inspection)

  • The two-worktree reproduction, done. /tmp/impl-99 (clean) and
    /tmp/impl-99-dirty (identical tree plus one file carrying a revive
    and an unused finding), make lint started in both at once. Clean:
    0 issues., exit 0. Dirty: exactly 2 findings, both citing
    internal/blobgen/lintbait.go in its own tree, exit 2. Neither log
    contained parallel golangci-lint is running. No leakage in either
    direction.
  • Foreign-path guard, end to end: a real script/lint run in the
    dirty worktree made to report outside paths reproduced this issue's
    signature (../impl-91/internal/blobgen/lintbait.go) and exited 1
    with the REJECTED diagnostic rather than reporting the findings. The
    auditor was additionally run against the verbatim output quoted in
    this issue (exit 1), a foreign path with no .. component (exit 1), a
    real clean capture (exit 0), and a container-absolute /src/... path
    (exit 0).
  • Warm lint 2.9s before, 4.3s after (cold 1m44s to 1m24s); the extra is
    the capture plus the audit scan. make check exit 0 with 14 ok
    lines, zero (cached), 0 issues.. script/cibuild exit 0.

One limitation, stated rather than papered over: script/lint
bind-mounts every tree at /src, so two containerized runs of different
checkouts both call themselves /src and contamination between them is
not distinguishable by path alone. That is why the per-worktree cache
key, not the audit, is the primary fix; the audit catches the signature
observed here and any host-side path leakage. It is documented at the
top of script/lint-audit.

Implemented in PR #102 (branch `fix-lint-isolation`, one commit, together with #80 since they are the same defect). **What was built** 1. The cache is now keyed per worktree: `${XDG_CACHE_HOME:-~/.cache}/vaultik-lint/<slug>-<12 hex of a digest of $ROOT>`. Still persistent, so the warm-run speed the shared cache existed for is kept; a collision between two checkouts is no longer possible. 2. New `script/lint-audit`, run on every lint, rejects output citing any file that is not in the tree being linted — absolute paths outside the root, `..` components, and relative paths that do not exist here. It runs on clean output too, since the unearned-**pass** direction is the silent one. It deliberately never certifies that a run passed: it does not look at whether there were findings, so it cannot become a second gate that reports a green. It is a separate script rather than an inline function so it can be exercised directly against a saved capture, which is how it was verified. 3. `parallel golangci-lint is running` is now a bounded retry (6 attempts, 15s apart), not a verdict. Exhausting the retries fails with a message stating the tree was never analysed. Confirmed the #88 reading that this is orthogonal to cache privacy. `--allow-parallel-runners` was rejected as the alternative: it disables the lock that protects a cache shared by two runs of the *same* worktree, trading a delay for corruption. 4. Each cache records the worktree it belongs to and is collected on the next run once that worktree is gone, so throwaway worktrees do not accumulate caches. The whole tree is under `XDG_CACHE_HOME` and disposable; the removal command appears in the script comment and in the rejection message. **How it was verified** (reproduction, not inspection) * **The two-worktree reproduction, done.** `/tmp/impl-99` (clean) and `/tmp/impl-99-dirty` (identical tree plus one file carrying a `revive` and an `unused` finding), `make lint` started in both at once. Clean: `0 issues.`, exit 0. Dirty: exactly 2 findings, both citing `internal/blobgen/lintbait.go` in its own tree, exit 2. Neither log contained `parallel golangci-lint is running`. No leakage in either direction. * **Foreign-path guard, end to end**: a real `script/lint` run in the dirty worktree made to report outside paths reproduced this issue's signature (`../impl-91/internal/blobgen/lintbait.go`) and exited **1** with the REJECTED diagnostic rather than reporting the findings. The auditor was additionally run against the verbatim output quoted in this issue (exit 1), a foreign path with no `..` component (exit 1), a real clean capture (exit 0), and a container-absolute `/src/...` path (exit 0). * Warm lint 2.9s before, 4.3s after (cold 1m44s to 1m24s); the extra is the capture plus the audit scan. `make check` exit 0 with 14 `ok` lines, zero `(cached)`, `0 issues.`. `script/cibuild` exit 0. **One limitation, stated rather than papered over**: `script/lint` bind-mounts every tree at `/src`, so two containerized runs of different checkouts both call themselves `/src` and contamination between them is not distinguishable by path alone. That is why the per-worktree cache key, not the audit, is the primary fix; the audit catches the signature observed here and any host-side path leakage. It is documented at the top of `script/lint-audit`.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
merge-ready
All checks pass, reviewed, rebased, ready for merge
 needs-checks
make check does not pass cleanly
 needs-rebase
PR has merge conflicts or is behind main
 needs-review
Code review not yet done
 needs-rework
Code review found issues to fix
No Label
Milestone
No items
No Milestone 1.0.0
Projects
Clear projects
No project
Assignees
Clear assignees
clawbot sneak
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/vaultik#99
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?