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

Two fail-closed defects in the new lint isolation #103

New Issue
Closed
opened 2026-08-09 17:15:23 +02:00 by clawbot · 1 comment
clawbot commented 2026-08-09 17:15:23 +02:00
Collaborator
Copy Link
Copy Source

Non-blocking findings from the PR #102
review. Both fail closed — they can abort or misdiagnose a lint run,
never report a false green — which is why they did not block the merge.
Both are one-to-three-line fixes.

1. script/lint — prune_dead_caches can still fail a lint

script/lint:174:

owner="$(cat "$dir/worktree")"

The only unguarded command in the function. Under set -eu a failed
command substitution in an assignment aborts the script, so a stale cache
with an unreadable marker makes ./script/lint exit 1 printing only
cat: .../worktree: Permission denied — without linting anything.

This contradicts the intent recorded in the PR, that tidy-up can never
fail a lint. It is the same class of defect the author already caught and
fixed once during implementation (an rm -rf on a read-only Go module
cache aborting the script), surviving in one remaining line.

Realistic trigger is two concurrent lints collecting the same stale
cache: [ -f ] passes, then A's rm -rf unlinks the marker before B's
cat. The reviewer could not win that race in 36 concurrent attempts, so
it is narrow.

Fix: 2>/dev/null || true on the substitution. The existing
[ -z "$owner" ] branch already handles the empty result.

2. script/lint-audit — dependency paths misdiagnosed as contamination

script/lint-audit:63-66 classes any absolute path outside $ROOT or
/src as foreign. Inside the container GOMODCACHE=/cache/go-mod, so a
legitimate typecheck finding citing a dependency's source is rejected
as cache contamination, telling the user to clear their lint cache when
the real problem is a dependency error.

Fix: exempt /cache/go-mod and /cache/go-build from the foreign
set. Keep everything else rejecting — the guard's value is that it
refuses to certify a pass.

Definition of done

  1. Both fixed.
  2. Verified by negative control, not inspection: a stale cache with an
    unreadable marker lints normally, and a capture containing a
    /cache/go-mod/... path no longer produces the contamination message.
  3. script/lint-audit still rejects the original #99
    signature and still never certifies a pass.
  4. script/cibuild exits 0.

Also worth correcting

The PR #102 body and its
issue comment state the dirty worktree exited 2; the reviewer observed
1, golangci-lint's default issues-exit-code. Minor, but the number
is quoted as evidence.

Non-blocking findings from the [PR #102](https://git.eeqj.de/sneak/vaultik/pulls/102) review. Both fail **closed** — they can abort or misdiagnose a lint run, never report a false green — which is why they did not block the merge. Both are one-to-three-line fixes. ## 1. `script/lint` — `prune_dead_caches` can still fail a lint `script/lint:174`: ```sh owner="$(cat "$dir/worktree")" ``` The only unguarded command in the function. Under `set -eu` a failed command substitution in an assignment aborts the script, so a stale cache with an unreadable marker makes `./script/lint` exit 1 printing only `cat: .../worktree: Permission denied` — without linting anything. This contradicts the intent recorded in the PR, that tidy-up can never fail a lint. It is the same class of defect the author already caught and fixed once during implementation (an `rm -rf` on a read-only Go module cache aborting the script), surviving in one remaining line. Realistic trigger is two concurrent lints collecting the same stale cache: `[ -f ]` passes, then A's `rm -rf` unlinks the marker before B's `cat`. The reviewer could not win that race in 36 concurrent attempts, so it is narrow. **Fix:** `2>/dev/null || true` on the substitution. The existing `[ -z "$owner" ]` branch already handles the empty result. ## 2. `script/lint-audit` — dependency paths misdiagnosed as contamination `script/lint-audit:63-66` classes any absolute path outside `$ROOT` or `/src` as foreign. Inside the container `GOMODCACHE=/cache/go-mod`, so a legitimate `typecheck` finding citing a dependency's source is rejected as cache contamination, telling the user to clear their lint cache when the real problem is a dependency error. **Fix:** exempt `/cache/go-mod` and `/cache/go-build` from the foreign set. Keep everything else rejecting — the guard's value is that it refuses to certify a pass. ## Definition of done 1. Both fixed. 2. Verified by negative control, not inspection: a stale cache with an unreadable marker lints normally, and a capture containing a `/cache/go-mod/...` path no longer produces the contamination message. 3. `script/lint-audit` still rejects the original [#99](https://git.eeqj.de/sneak/vaultik/issues/99) signature and still never certifies a pass. 4. `script/cibuild` exits 0. ## Also worth correcting The [PR #102](https://git.eeqj.de/sneak/vaultik/pulls/102) body and its issue comment state the dirty worktree exited **2**; the reviewer observed **1**, golangci-lint's default `issues-exit-code`. Minor, but the number is quoted as evidence.
clawbot added this to the 1.0.0 milestone 2026-08-09 17:15:23 +02:00
clawbot commented 2026-08-11 13:55:37 +02:00
Author
Collaborator
Copy Link
Copy Source

Obsolete: both defects were in code deleted on next (d257f8f, issue #113). Defect 1's prune_dead_caches is gone along with the whole per-worktree cache mechanism — a container per lint run has its own cache, so there is nothing to prune. Defect 2's script/lint-audit is gone entirely; contamination cannot occur, so there is nothing to audit for. Neither script/lint-audit nor prune_dead_caches exists on next.

The exit-code correction (1, not 2, being golangci-lint's default issues-exit-code) is noted; pull request #102 is merged and its body is history. Closing.

Obsolete: both defects were in code deleted on `next` (`d257f8f`, [issue #113](https://git.eeqj.de/sneak/vaultik/issues/113)). Defect 1's `prune_dead_caches` is gone along with the whole per-worktree cache mechanism — a container per lint run has its own cache, so there is nothing to prune. Defect 2's `script/lint-audit` is gone entirely; contamination cannot occur, so there is nothing to audit for. Neither `script/lint-audit` nor `prune_dead_caches` exists on `next`. The exit-code correction (1, not 2, being golangci-lint's default `issues-exit-code`) is noted; [pull request #102](https://git.eeqj.de/sneak/vaultik/pulls/102) is merged and its body is history. Closing.
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 labels
Milestone
No items
No Milestone 1.0.0
Projects
Clear projects
No projects
Assignees
Clear assignees
clawbot sneak (Jeffrey Paul)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/vaultik#103