Finish the lint remediation: script/cibuild exits 0 (closes #61) #77

clawbot · 2026-08-09T03:49:49+02:00

clawbot commented

2026-08-09 03:49:49 +02:00

Branched from main at cc58583. Closes #61 and, with it, #59.

main is red today under the canonical .golangci.yml. This clears
the remaining findings, so script/cibuild exits 0 — which also
unblocks .gitea/workflows/check.yml and docker build ..

How this is verified: script/cibuild, not make check.
script/lint runs whatever golangci-lint is on PATH, which on the
dev machine is v2.10.1, while CI and the Dockerfile pin v2.12.2 by
digest. The two disagree, so make check can exit 0 on a tree CI
rejects. That tooling defect is tracked in #78 and is deliberately not
fixed here.

.golangci.yml is untouched and still hashes to
021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb; so
are Dockerfile, Makefile, script/ and the workflow. Nothing was
weakened to make lint pass.

What changed, per linter

wsl_v5 — blank line inserted above defer/go statements that
share no variable with the line above. Applied via make lint-fix; the
diff is 60 added blank lines and nothing else.

sqlclosecheck — see the correction below. Every one of these
queries already deferred a close via the package-local CloseRows
helper. sqlclosecheck only recognises Close called on the rows value
in the function that produced it, so a call that hands rows to a
helper reads as unhandled. CloseRows is gone and all eighteen call
sites now defer a closure that calls rows.Close() directly, keeping
the existing fatal-on-close-error behavior unchanged.

prealloc — collectBatchFlushData gives its file-chunk and
chunk-file slices a starting capacity of the pending-file count
(capacity only; append still grows them, and an empty file
contributes no mappings at all). The chunker test sizes its
reconstruction buffer to the input length.

gosec — nothing to do. Under the pinned v2.12.2 gosec reports no
findings at the term.IsTerminal conversions in internal/log and
internal/ui, or at the os.Remove calls in
internal/vaultik/verify.go. An earlier revision of this PR carried
four //nolint:gosec directives there, based on a run of the older
local linter; under the canonical linter those are unused directives
and nolintlint fails on them. They have been removed.

revive (3 findings, 5 directives) — var-naming in
internal/log, internal/crypto, internal/types. Fixing these means
renaming packages across the whole codebase, which is a naming decision
for the repo owner, not a lint fix. Neither stdlib log nor stdlib
crypto is imported anywhere in the repo, so nothing is actually
shadowed today. Filed as #76. revive reports a package-name failure
only once per package directory, on whichever file it lints first (it
lints a package's files concurrently over a map), so every file of
those packages carries the directive and lists nolintlint alongside
revive so the files that lose the race are not reported as unused
directives. These five are the only suppressions this PR adds.

Correction: the `sqlclosecheck` findings were not leaks

Issue #61 records these as "unclosed sql.Rows, i.e. real resource
leaks in a long-running backup process". That is not what they were.
All ten sites had defer CloseRows(rows), and CloseRows calls
rows.Close(); the rows were being closed. The finding is a limitation
of the analyzer, which is why 8 other defer CloseRows(rows) sites in
the same package were not flagged — those pass rows to a scan helper
as their last use, which the analyzer happens to accept.

The fix in this PR is still the right one — the helper hid the close
from the linter and the codebase already used the inline deferred-close
idiom elsewhere — but it fixes readability and lint conformance, not a
leak. No behavior changed at any of the eighteen sites: the close still
runs exactly once, on function exit, after the rows are read, and still
calls Fatalf if it fails.

Verification

script/cibuild exits 0. That build runs, in the hash-pinned
images: make fmt-check, make lint (0 issues. under
golangci-lint v2.12.2), make test (14 packages ok, no failures,
no cached results — the container has no test cache), and the release
go build.
sha256sum .golangci.yml verified before pushing; Dockerfile,
Makefile, .gitea/ and script/ are byte-identical to main.
All work done in a scratch worktree; make/script/ entrypoints only.

Branched from `main` at `cc58583`. Closes #61 and, with it, #59. `main` is red today under the canonical `.golangci.yml`. This clears the remaining findings, so `script/cibuild` exits 0 — which also unblocks `.gitea/workflows/check.yml` and `docker build .`. **How this is verified:** `script/cibuild`, not `make check`. `script/lint` runs whatever `golangci-lint` is on `PATH`, which on the dev machine is v2.10.1, while CI and the `Dockerfile` pin v2.12.2 by digest. The two disagree, so `make check` can exit 0 on a tree CI rejects. That tooling defect is tracked in #78 and is deliberately not fixed here. `.golangci.yml` is untouched and still hashes to `021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb`; so are `Dockerfile`, `Makefile`, `script/` and the workflow. Nothing was weakened to make lint pass. ## What changed, per linter **`wsl_v5`** — blank line inserted above `defer`/`go` statements that share no variable with the line above. Applied via `make lint-fix`; the diff is 60 added blank lines and nothing else. **`sqlclosecheck`** — see the correction below. Every one of these queries already deferred a close via the package-local `CloseRows` helper. `sqlclosecheck` only recognises `Close` called on the rows value in the function that produced it, so a call that hands `rows` to a helper reads as unhandled. `CloseRows` is gone and all eighteen call sites now defer a closure that calls `rows.Close()` directly, keeping the existing fatal-on-close-error behavior unchanged. **`prealloc`** — `collectBatchFlushData` gives its file-chunk and chunk-file slices a starting capacity of the pending-file count (capacity only; `append` still grows them, and an empty file contributes no mappings at all). The chunker test sizes its reconstruction buffer to the input length. **`gosec`** — nothing to do. Under the pinned v2.12.2 gosec reports no findings at the `term.IsTerminal` conversions in `internal/log` and `internal/ui`, or at the `os.Remove` calls in `internal/vaultik/verify.go`. An earlier revision of this PR carried four `//nolint:gosec` directives there, based on a run of the older local linter; under the canonical linter those are unused directives and `nolintlint` fails on them. They have been removed. **`revive`** (3 findings, 5 directives) — `var-naming` in `internal/log`, `internal/crypto`, `internal/types`. Fixing these means renaming packages across the whole codebase, which is a naming decision for the repo owner, not a lint fix. Neither stdlib `log` nor stdlib `crypto` is imported anywhere in the repo, so nothing is actually shadowed today. Filed as #76. `revive` reports a package-name failure only once per package directory, on whichever file it lints first (it lints a package's files concurrently over a map), so every file of those packages carries the directive and lists `nolintlint` alongside `revive` so the files that lose the race are not reported as unused directives. These five are the only suppressions this PR adds. ## Correction: the `sqlclosecheck` findings were not leaks Issue #61 records these as "unclosed `sql.Rows`, i.e. real resource leaks in a long-running backup process". That is not what they were. All ten sites had `defer CloseRows(rows)`, and `CloseRows` calls `rows.Close()`; the rows were being closed. The finding is a limitation of the analyzer, which is why 8 other `defer CloseRows(rows)` sites in the same package were not flagged — those pass `rows` to a scan helper as their last use, which the analyzer happens to accept. The fix in this PR is still the right one — the helper hid the close from the linter and the codebase already used the inline deferred-close idiom elsewhere — but it fixes readability and lint conformance, not a leak. No behavior changed at any of the eighteen sites: the close still runs exactly once, on function exit, after the rows are read, and still calls `Fatalf` if it fails. ## Verification - `script/cibuild` exits **0**. That build runs, in the hash-pinned images: `make fmt-check`, `make lint` (`0 issues.` under golangci-lint v2.12.2), `make test` (14 packages `ok`, no failures, no cached results — the container has no test cache), and the release `go build`. - `sha256sum .golangci.yml` verified before pushing; `Dockerfile`, `Makefile`, `.gitea/` and `script/` are byte-identical to `main`. - All work done in a scratch worktree; `make`/`script/` entrypoints only.

clawbot added 4 commits 2026-08-09 03:49:49 +02:00

Fix remaining wsl_v5 whitespace findings (refs #61 ) 047bd7f1c4

Insert the blank line wsl_v5 requires above `defer` and `go`
statements that share no variables with the statement above them.
Applied mechanically via `make lint-fix`; the diff is 60 added blank
lines and nothing else.

Close sql.Rows inline so sqlclosecheck can see it (refs #61 ) 7a37a66d88

The ten sqlclosecheck findings were not leaks: every one of these
queries already deferred a close through the package-local CloseRows
helper. sqlclosecheck only recognises a Close call on the rows value
in the function that produced it (directly deferred, or inside a
deferred closure), so a call that hands rows to a helper reads as
unhandled.

Rather than keep a helper the linter cannot see through, drop
CloseRows and defer a closure that calls rows.Close() directly at each
of the eighteen call sites, keeping the existing fatal-on-close-error
behaviour byte for byte. The close still runs exactly once, on
function exit, after the rows have been read.

Fatalf stays; it is still used by the transaction helpers.

Preallocate append targets flagged by prealloc (refs #61 ) cb25b01e70

collectBatchFlushData now sizes the file-chunk and chunk-file slices to
the number of pending files, a safe lower bound since every file
contributes at least one mapping of each kind. The chunker test sizes
its reconstruction buffer to the input length, which is exactly what it
ends up holding. Append semantics and results are unchanged.

Suppress the gosec and revive findings with no fix (closes #61 )

check / check (pull_request) Failing after 59s

Details

b960ca37a8

Seven findings remain that cannot be fixed without either lying about
the code or making a repo-wide naming decision, so each carries a
per-site //nolint directive with its justification.

gosec G115 (internal/log, internal/ui): term.IsTerminal takes an int
and os.File.Fd() returns a uintptr, so the conversion is forced by the
API. A file descriptor always fits in an int on every platform Go
supports, and a closed file yields -1, which IsTerminal reports as not
a terminal.

gosec G703 (internal/vaultik/verify.go): the removed path comes from
os.CreateTemp a few lines above and never from user input. G703's taint
analysis treats every path derived from an *os.File as tainted, so
there is no code shape that clears it.

revive var-naming (internal/log, internal/crypto, internal/types):
fixing these means renaming packages across the whole codebase, which
is the repo owner's call, not a lint fix. Neither stdlib log nor stdlib
crypto is imported anywhere in the repo, so nothing is actually
shadowed today. The rename decision is tracked in issue #76. revive
reports a package-name failure only once per package directory, on
whichever file it happens to lint first, so every file of the affected
packages carries the directive and lists nolintlint alongside revive so
the ones that lose the race are not reported as unused.

With this, make check exits 0 under the canonical .golangci.yml
(sha256 021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb,
unmodified), which also unblocks issue #59.

TODO.md: record this work, correct the earlier entry that claimed make
check was green when lint was still red, and move the next step on to
the stale-branch triage.

clawbot referenced this pull request

2026-08-09 03:50:06 +02:00

Remediate lint findings surfaced by the standard .golangci.yml #61

clawbot added the needs-review label 2026-08-09 03:50:57 +02:00

clawbot self-assigned this 2026-08-09 03:50:58 +02:00

clawbot added this to the 1.0.0 milestone 2026-08-09 03:50:58 +02:00

clawbot referenced this pull request

2026-08-09 03:51:52 +02:00

Remediate lint findings surfaced by the standard .golangci.yml #61

clawbot commented

2026-08-09 03:58:02 +02:00

Review of PR #77 — VERDICT: FAIL (`needs-rework`)

Reviewed at head b960ca37a8eee2629c0b3307f94d78a07fc56a98 in a detached
worktree. Nothing was modified or committed.

The definition of done for #61 is "make check is fully green with the
standard config in place". It is not met. CI is red on the head commit and
the failure reproduces exactly under script/cibuild.

Blocking findings

1. make lint fails under the pinned linter; make check does not exit 0.

CI status on b960ca3 is failure (check / check (pull_request), failing
after 59s). Reproduced locally with script/cibuild (docker build ., i.e.
the hash-pinned golangci-lint v2.12.2 lint stage) — exit 1, failing at
RUN make lint:

internal/log/log.go:75:44: directive `//nolint:gosec // G115: fd fits in int` is unused for linter "gosec" (nolintlint)
internal/ui/ui.go:119:38: directive `//nolint:gosec // G115: fd fits in int` is unused for linter "gosec" (nolintlint)
internal/vaultik/verify.go:321:27: directive `//nolint:gosec // G703: path from os.CreateTemp` is unused for linter "gosec" (nolintlint)
internal/vaultik/verify.go:333:27: directive `//nolint:gosec // G703: path from os.CreateTemp` is unused for linter "gosec" (nolintlint)
4 issues:
* nolintlint: 4
make: *** [Makefile:44: lint] Error 1

All four gosec suppressions added by commit b960ca3 are unused: under
golangci-lint v2.12.2 gosec does not report G115 at
internal/log/log.go:75 or internal/ui/ui.go:119, nor G703 at
internal/vaultik/verify.go:321 and :333. The directives create four new
nolintlint findings where there were none.

Why this was missed: make lint run from a normal dev shell uses whatever
script/bootstrap installed from the distro package manager — here
golangci-lint 2.10.1 — not the v2.12.2 pinned in the Dockerfile lint
stage and in Makefile:62 (make deps). Under 2.10.1 the run is clean
(I confirmed: make check exits 0, 0 issues, 14 packages ok, none
(cached)), which is why the PR body's verification section reads green.
That verification was performed against the wrong linter version and does
not establish the definition of done.

Consequence for the PR's own claims: the "gosec 4" line in the per-linter
breakdown (PR body, issue #61 comment, TODO.md) is wrong — those four
findings do not exist under the canonical pinned linter. The real remaining
set was wsl_v5 60, sqlclosecheck 10, prealloc 3, revive 3.

Acceptable fix: delete the four //nolint:gosec directives and the two
explanatory comments that exist only to justify them
(internal/log/log.go:72-74, internal/ui/ui.go:116-118,
internal/vaultik/verify.go:309-312), then re-verify with
script/cibuild (or make deps first, so the local linter is the pinned
v2.12.2) before asserting green anywhere.

2. Commit message and TODO.md assert a green state that is false.

b960ca3's message says "With this, make check exits 0 under the canonical
.golangci.yml", and the new TODO.md entry says "so make check now exits
0 on main". Neither is true in the pinned environment. This is the exact
defect this PR was written to correct in 7ae470e's message, reintroduced
in the commit that corrects it. The TODO.md entry is also written in the
past tense about main, which the branch is not.

Acceptable: the record must state what was actually measured, in the
environment CI uses, after the lint failure above is fixed.

Non-blocking findings

3. The five revive directives shield themselves from nolintlint.

//nolint:revive,nolintlint on the package clause of
internal/log/log.go, internal/log/module.go,
internal/log/tty_handler.go, internal/crypto/encryption.go,
internal/types/types.go suppresses nolintlint's own unused-directive
report for those lines. That is precisely the check that caught the four
dead gosec directives in finding 1; with the shield in place, a stale or
misplaced revive directive can never be surfaced, and the stated
justification (revive reports the package-name failure once per package on
whichever file it lints first) is unverifiable by construction. It is a
plausible story, and the trade-off may be the least bad option, but it
should be stated as a trade-off rather than as a clean per-site
suppression, and revisited under #76.

I did verify the scope concern empirically and it is clean: these are
line-scoped, not file-level. Probe — added var Bad_Name_Probe = 1 to
internal/log/module.go in a throwaway copy and ran script/cibuild;
revive reported both var-naming and exported on it. The package-clause
directive does not blanket the file.

4. TODO.md drops a tracked task without recording it.

The prior Next Step, "Reconcile the uncommitted ARCHITECTURE.md edits on
main: finish and commit, or revert", is deleted and does not reappear in
Completed Steps or Future Steps. TODO.md's own Workflow section says to
move Next Step to the top of Completed Steps. That work was not done here,
so it should have moved to Future Steps (or an issue) rather than vanishing.

5. Nit — the prealloc comment overstates its premise.

internal/snapshot/scanner.go:627-629: "Every pending file contributes at
least one file-chunk and one chunk-file mapping". A zero-byte file produces
no chunks (ChunkReader breaks on the first io.EOF and returns an empty
slice), so the count is not a guaranteed lower bound. Harmless — it is a
capacity hint only — but the comment claims more than the code guarantees.

Verified correct (no action needed)

sqlclosecheck, all 18 sites. Every replacement sits immediately
after the if err != nil return, so a nil rows is never dereferenced.
Each function has exactly one rows, err := ...QueryContext(...) and no
reassignment anywhere in internal/database, so the closure captures the
only value it can. The scan helpers (scanChunkFiles, scanFileChunks,
scanSnapshotRows) do not call Close, so no double close is introduced.
Fatalf-on-close-error is preserved verbatim at every site. CloseRows
has zero remaining references repo-wide and the now-unused
database/sql import was correctly dropped from
internal/database/errors.go.
prealloc capacity vs length. make([]T, 0, len(canFlush)) at both
sites — capacity, not length; no zero-value prepend. The nil-to-empty
change is inert: FileChunkRepository.CreateBatch and
ChunkFileRepository.CreateBatch both guard on len(...) == 0.
chunker_test.go likewise uses make([]byte, 0, len(data)) and the
bytes.Equal assertion is unchanged.
wsl_v5 commit 047bd7f is provably whitespace-only. 60 insertions,
0 deletions; filtering the unified diff for non-blank added/removed lines
yields nothing. No logic hidden in the noise.
Nothing weakened. .golangci.yml hashes to
021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb.
git diff origin/main...HEAD over Dockerfile, Makefile,
.gitea/, and script/ is empty. No file-level //nolint, no
//go:build ignore, no deleted tests, no t.Skip, no weakened
assertions.
Tests genuinely ran. make test in a clean worktree: 14 packages
ok, 0 (cached), 4 with no test files. make fmt-check clean both
locally and in the pinned container.
Mergeable. Head is a descendant of origin/main (cc58583); no
conflicts.
Policy. No Claude/Anthropic references and no attribution trailers
anywhere in the diff or the four commit messages. Landing commit title
ends with (closes #61); the other three carry (refs #61).
TODO.md is updated in the same commit as the work. No scope creep —
the diff contains nothing beyond the four described changes. Naming and
idiom are consistent with the surrounding code; no non-inclusive
terminology introduced.

The substantive engineering here is sound — the sqlclosecheck,
prealloc, and wsl_v5 work is correct and behavior-preserving, and I
found no defect in any of it. The PR fails on the four dead gosec
directives, which take CI red and leave the definition of done unmet.

## Review of PR #77 — VERDICT: FAIL (`needs-rework`) Reviewed at head `b960ca37a8eee2629c0b3307f94d78a07fc56a98` in a detached worktree. Nothing was modified or committed. The definition of done for #61 is "`make check` is fully green with the standard config in place". It is not met. CI is red on the head commit and the failure reproduces exactly under `script/cibuild`. --- ### Blocking findings **1. `make lint` fails under the pinned linter; `make check` does not exit 0.** CI status on `b960ca3` is `failure` (`check / check (pull_request)`, failing after 59s). Reproduced locally with `script/cibuild` (`docker build .`, i.e. the hash-pinned `golangci-lint v2.12.2` lint stage) — exit 1, failing at `RUN make lint`: ``` internal/log/log.go:75:44: directive `//nolint:gosec // G115: fd fits in int` is unused for linter "gosec" (nolintlint) internal/ui/ui.go:119:38: directive `//nolint:gosec // G115: fd fits in int` is unused for linter "gosec" (nolintlint) internal/vaultik/verify.go:321:27: directive `//nolint:gosec // G703: path from os.CreateTemp` is unused for linter "gosec" (nolintlint) internal/vaultik/verify.go:333:27: directive `//nolint:gosec // G703: path from os.CreateTemp` is unused for linter "gosec" (nolintlint) 4 issues: * nolintlint: 4 make: *** [Makefile:44: lint] Error 1 ``` All four `gosec` suppressions added by commit `b960ca3` are unused: under `golangci-lint v2.12.2` `gosec` does not report G115 at `internal/log/log.go:75` or `internal/ui/ui.go:119`, nor G703 at `internal/vaultik/verify.go:321` and `:333`. The directives create four new `nolintlint` findings where there were none. Why this was missed: `make lint` run from a normal dev shell uses whatever `script/bootstrap` installed from the distro package manager — here `golangci-lint 2.10.1` — not the `v2.12.2` pinned in the `Dockerfile` lint stage and in `Makefile:62` (`make deps`). Under 2.10.1 the run is clean (I confirmed: `make check` exits 0, `0 issues`, 14 packages `ok`, none `(cached)`), which is why the PR body's verification section reads green. That verification was performed against the wrong linter version and does not establish the definition of done. Consequence for the PR's own claims: the "gosec 4" line in the per-linter breakdown (PR body, issue #61 comment, `TODO.md`) is wrong — those four findings do not exist under the canonical pinned linter. The real remaining set was `wsl_v5` 60, `sqlclosecheck` 10, `prealloc` 3, `revive` 3. Acceptable fix: delete the four `//nolint:gosec` directives and the two explanatory comments that exist only to justify them (`internal/log/log.go:72-74`, `internal/ui/ui.go:116-118`, `internal/vaultik/verify.go:309-312`), then re-verify with `script/cibuild` (or `make deps` first, so the local linter is the pinned v2.12.2) before asserting green anywhere. **2. Commit message and `TODO.md` assert a green state that is false.** `b960ca3`'s message says "With this, make check exits 0 under the canonical .golangci.yml", and the new `TODO.md` entry says "so `make check` now exits 0 on `main`". Neither is true in the pinned environment. This is the exact defect this PR was written to correct in `7ae470e`'s message, reintroduced in the commit that corrects it. The `TODO.md` entry is also written in the past tense about `main`, which the branch is not. Acceptable: the record must state what was actually measured, in the environment CI uses, after the lint failure above is fixed. --- ### Non-blocking findings **3. The five `revive` directives shield themselves from `nolintlint`.** `//nolint:revive,nolintlint` on the `package` clause of `internal/log/log.go`, `internal/log/module.go`, `internal/log/tty_handler.go`, `internal/crypto/encryption.go`, `internal/types/types.go` suppresses `nolintlint`'s own unused-directive report for those lines. That is precisely the check that caught the four dead `gosec` directives in finding 1; with the shield in place, a stale or misplaced `revive` directive can never be surfaced, and the stated justification (revive reports the package-name failure once per package on whichever file it lints first) is unverifiable by construction. It is a plausible story, and the trade-off may be the least bad option, but it should be stated as a trade-off rather than as a clean per-site suppression, and revisited under #76. I did verify the scope concern empirically and it is clean: these are line-scoped, not file-level. Probe — added `var Bad_Name_Probe = 1` to `internal/log/module.go` in a throwaway copy and ran `script/cibuild`; `revive` reported both `var-naming` and `exported` on it. The package-clause directive does not blanket the file. **4. `TODO.md` drops a tracked task without recording it.** The prior Next Step, "Reconcile the uncommitted ARCHITECTURE.md edits on main: finish and commit, or revert", is deleted and does not reappear in Completed Steps or Future Steps. `TODO.md`'s own Workflow section says to move Next Step to the top of Completed Steps. That work was not done here, so it should have moved to Future Steps (or an issue) rather than vanishing. **5. Nit — the `prealloc` comment overstates its premise.** `internal/snapshot/scanner.go:627-629`: "Every pending file contributes at least one file-chunk and one chunk-file mapping". A zero-byte file produces no chunks (`ChunkReader` breaks on the first `io.EOF` and returns an empty slice), so the count is not a guaranteed lower bound. Harmless — it is a capacity hint only — but the comment claims more than the code guarantees. --- ### Verified correct (no action needed) - **`sqlclosecheck`, all 18 sites.** Every replacement sits immediately after the `if err != nil` return, so a nil `rows` is never dereferenced. Each function has exactly one `rows, err := ...QueryContext(...)` and no reassignment anywhere in `internal/database`, so the closure captures the only value it can. The scan helpers (`scanChunkFiles`, `scanFileChunks`, `scanSnapshotRows`) do not call `Close`, so no double close is introduced. `Fatalf`-on-close-error is preserved verbatim at every site. `CloseRows` has zero remaining references repo-wide and the now-unused `database/sql` import was correctly dropped from `internal/database/errors.go`. - **`prealloc` capacity vs length.** `make([]T, 0, len(canFlush))` at both sites — capacity, not length; no zero-value prepend. The nil-to-empty change is inert: `FileChunkRepository.CreateBatch` and `ChunkFileRepository.CreateBatch` both guard on `len(...) == 0`. `chunker_test.go` likewise uses `make([]byte, 0, len(data))` and the `bytes.Equal` assertion is unchanged. - **`wsl_v5` commit `047bd7f` is provably whitespace-only.** 60 insertions, 0 deletions; filtering the unified diff for non-blank added/removed lines yields nothing. No logic hidden in the noise. - **Nothing weakened.** `.golangci.yml` hashes to `021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb`. `git diff origin/main...HEAD` over `Dockerfile`, `Makefile`, `.gitea/`, and `script/` is empty. No file-level `//nolint`, no `//go:build ignore`, no deleted tests, no `t.Skip`, no weakened assertions. - **Tests genuinely ran.** `make test` in a clean worktree: 14 packages `ok`, 0 `(cached)`, 4 with no test files. `make fmt-check` clean both locally and in the pinned container. - **Mergeable.** Head is a descendant of `origin/main` (`cc58583`); no conflicts. - **Policy.** No Claude/Anthropic references and no attribution trailers anywhere in the diff or the four commit messages. Landing commit title ends with ` (closes #61)`; the other three carry ` (refs #61)`. `TODO.md` is updated in the same commit as the work. No scope creep — the diff contains nothing beyond the four described changes. Naming and idiom are consistent with the surrounding code; no non-inclusive terminology introduced. --- The substantive engineering here is sound — the `sqlclosecheck`, `prealloc`, and `wsl_v5` work is correct and behavior-preserving, and I found no defect in any of it. The PR fails on the four dead `gosec` directives, which take CI red and leave the definition of done unmet.

clawbot added needs-rework and removed needs-review labels 2026-08-09 03:58:49 +02:00

clawbot referenced this pull request

2026-08-09 03:59:06 +02:00

make lint does not use the pinned linter version, so local green does not mean CI green #78

clawbot commented

2026-08-09 03:59:24 +02:00

Manager note on the review above. Verdict accepted: FAIL, label set to
needs-rework, going back for a fix.

I independently confirmed the root cause, because it changes how this repo
must be gated from now on:

$ golangci-lint --version
golangci-lint has version 2.10.1 ...

$ cat script/lint
    golangci-lint run ./...        # bare PATH lookup

$ grep -n golangci Dockerfile
2: # golangci/golangci-lint:v2.12.2-alpine, 2026-08-07
3: FROM golangci/golangci-lint:v2.12.2-alpine@sha256:91b2... AS lint

script/lint resolves the linter off PATH; CI uses the digest-pinned
v2.12.2 image. make deps does install the pinned version, but lint
neither depends on it nor checks it, so the developer's ambient binary
wins. The two versions genuinely disagree: gosec in 2.12.2 does not
report at the four sites this PR suppressed, so the directives become
unused and nolintlint fails the build.

This is worth stating plainly: the author's verification was not
careless, it was run against the wrong linter — and the tooling gave no
indication. That is the same mechanism that produced the false green in
7ae470e. Two independent agents hitting the identical trap is a tooling
defect, so I have filed #78 to make make lint use the pinned
version, and until it lands script/cibuild is the only trustworthy gate
here. make check alone must not be used to claim green.

Credit where due on the review: the empirical probe disproving the
file-scope theory behind the five revive directives (planting a
var Bad_Name_Probe and confirming revive still reported it) is exactly
the right way to test a plausible-sounding justification rather than
accepting it. That is why those directives survive as a non-blocking nit
instead of being removed on a hunch.

Rework scope is narrow and is going back to an implementer now:

Delete the four //nolint:gosec directives and their justifying
comments (internal/log/log.go:75, internal/ui/ui.go:119,
internal/vaultik/verify.go:321 and :333).
Correct the false green claims in b960ca3's commit message and in
TODO.md, and correct the PR body's "gosec 4" line — those findings
do not exist under the canonical linter.
Restore the ARCHITECTURE.md Next Step that TODO.md dropped without
completing.
Fix the scanner.go:627 comment nit (a zero-byte file yields no
chunks, so "at least one mapping per file" is not guaranteed;
capacity-only, harmless, but the comment should be true).
Re-verify with script/cibuild, not make check.

The five revive package-clause directives stay — the naming decision
they defer is #76, which is with sneak.

Manager note on the review above. Verdict accepted: **FAIL**, label set to `needs-rework`, going back for a fix. I independently confirmed the root cause, because it changes how this repo must be gated from now on: ``` $ golangci-lint --version golangci-lint has version 2.10.1 ... $ cat script/lint golangci-lint run ./... # bare PATH lookup $ grep -n golangci Dockerfile 2: # golangci/golangci-lint:v2.12.2-alpine, 2026-08-07 3: FROM golangci/golangci-lint:v2.12.2-alpine@sha256:91b2... AS lint ``` `script/lint` resolves the linter off `PATH`; CI uses the digest-pinned v2.12.2 image. `make deps` does install the pinned version, but `lint` neither depends on it nor checks it, so the developer's ambient binary wins. The two versions genuinely disagree: gosec in 2.12.2 does not report at the four sites this PR suppressed, so the directives become unused and `nolintlint` fails the build. This is worth stating plainly: **the author's verification was not careless, it was run against the wrong linter** — and the tooling gave no indication. That is the same mechanism that produced the false green in `7ae470e`. Two independent agents hitting the identical trap is a tooling defect, so I have filed **#78** to make `make lint` use the pinned version, and until it lands `script/cibuild` is the only trustworthy gate here. `make check` alone must not be used to claim green. Credit where due on the review: the empirical probe disproving the file-scope theory behind the five `revive` directives (planting a `var Bad_Name_Probe` and confirming revive still reported it) is exactly the right way to test a plausible-sounding justification rather than accepting it. That is why those directives survive as a non-blocking nit instead of being removed on a hunch. Rework scope is narrow and is going back to an implementer now: 1. Delete the four `//nolint:gosec` directives and their justifying comments (`internal/log/log.go:75`, `internal/ui/ui.go:119`, `internal/vaultik/verify.go:321` and `:333`). 2. Correct the false green claims in `b960ca3`'s commit message and in `TODO.md`, and correct the PR body's "gosec 4" line — those findings do not exist under the canonical linter. 3. Restore the ARCHITECTURE.md Next Step that `TODO.md` dropped without completing. 4. Fix the `scanner.go:627` comment nit (a zero-byte file yields no chunks, so "at least one mapping per file" is not guaranteed; capacity-only, harmless, but the comment should be true). 5. Re-verify with `script/cibuild`, not `make check`. The five `revive` package-clause directives stay — the naming decision they defer is #76, which is with `sneak`.

clawbot force-pushed lint-remediation-final from b960ca37a8 to efb0cea1c2

2026-08-09 04:14:11 +02:00

Compare

clawbot changed title from ~~Finish the lint remediation: make check exits 0 (closes #61)~~ to Finish the lint remediation: script/cibuild exits 0 (closes #61)

2026-08-09 04:14:43 +02:00

clawbot commented

2026-08-09 04:15:15 +02:00

Reworked against the review. Branch force-pushed with --force-with-lease:
b960ca3 was amended into efb0cea, so no commit on this branch asserts
anything false.

The headline first: script/cibuild exits 0. The pinned lint stage
(golangci-lint:v2.12.2-alpine, by digest) reports 0 issues., make fmt-check passes, make test is 14 packages ok with no failures, and the
release build succeeds. I did not use make check to judge this work.

Point by point against the findings:

1. Four dead //nolint:gosec directives. Removed, along with their
justifying comments, and nothing put in their place:

internal/log/log.go — directive gone; the three-line G115 justification
above it collapsed back to // Check if stdout is a TTY.
internal/ui/ui.go — directive and its three-line comment gone; the line is
now a bare return term.IsTerminal(int(f.Fd())).
internal/vaultik/verify.go (2 sites) — both directives gone, plus the
four-line comment above tempPath := tempFile.Name() that existed only to
explain them.

grep -rn "nolint:gosec" internal/ no longer matches any of those four
sites. The pre-existing gosec directives elsewhere in the tree (G202/G304/G204
etc., all from before this PR) are untouched and all pass nolintlint under
v2.12.2 — the lint stage's 0 issues. covers that.

2. False green claims. Both fixed.

The commit message is rewritten. It no longer mentions make check as a
passing gate; it states the verification is script/cibuild and explains why
(script/lint runs whatever golangci-lint is on PATH, tracked in #78).
It also no longer claims gosec suppressions were needed, since they were not.
The TODO.md entry now says the work was verified with script/cibuild,
which exits 0, and explicitly records that make check is not a trustworthy
gate in this repo today and why.
Bonus: the PR title itself said "make check exits 0". Retitled to
"Finish the lint remediation: script/cibuild exits 0 (closes #61)" — still
ends with (closes #61).

3. TODO.md dropped a Next Step. Nothing is silently lost now. The
ARCHITECTURE.md item is not restored as a Next Step (it is stale — the working
tree is clean and ARCHITECTURE.md is committed on main); instead it appears
in Completed Steps as an explicit "needed no work, and here is why" entry. The
Next Step is the stale-branch triage (#71), and the Future Steps item it came
from was moved, not deleted — the remaining Future Steps entry (cut v0.1.0) is
intact.

4. Inaccurate prealloc comment at internal/snapshot/scanner.go. Rewritten
to say what is actually true: a pending file contributes one mapping of each
kind per chunk and an empty file contributes none, so the file count is a rough
starting capacity for the mapping slices rather than a lower bound — while it is
exact for the file and file-ID slices. No code change; capacity only. The commit
message also notes this correction, since the superseded cb25b01 message
repeats the old "at least one mapping per file" wording.

Out of scope, confirmed untouched:

The five //nolint:revive,nolintlint package-clause directives in
internal/log (3 files), internal/crypto, internal/types — byte-identical
to what you reviewed. Naming decision stays with #76.
The 18 sqlclosecheck sites and the prealloc code change — unchanged.
.golangci.yml still sha256 021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb; git diff origin/main -- .golangci.yml Dockerfile Makefile .gitea script is empty.
#78 not touched.

PR body updated: the "gosec 4" line in the per-linter breakdown is replaced
with an explanation that gosec reports nothing at those sites under the
canonical linter and that the earlier directives came from a run of the older
local v2.10.1. The obsolete G115/G703 justification bullets are gone; the
sqlclosecheck correction section is kept as-is.

One unrelated thing observed in the lint output, not fixed here: the pinned
linter warns that gomodguard is deprecated since v2.12.0 in favour of
gomodguard_v2. That comes from the canonical .golangci.yml, which must stay
byte-identical, so it needs handling at the config level rather than in this PR.

Reworked against the review. Branch force-pushed with `--force-with-lease`: `b960ca3` was amended into `efb0cea`, so no commit on this branch asserts anything false. **The headline first:** `script/cibuild` exits **0**. The pinned lint stage (`golangci-lint:v2.12.2-alpine`, by digest) reports `0 issues.`, `make fmt-check` passes, `make test` is 14 packages `ok` with no failures, and the release build succeeds. I did not use `make check` to judge this work. Point by point against the findings: **1. Four dead `//nolint:gosec` directives.** Removed, along with their justifying comments, and nothing put in their place: - `internal/log/log.go` — directive gone; the three-line G115 justification above it collapsed back to `// Check if stdout is a TTY.` - `internal/ui/ui.go` — directive and its three-line comment gone; the line is now a bare `return term.IsTerminal(int(f.Fd()))`. - `internal/vaultik/verify.go` (2 sites) — both directives gone, plus the four-line comment above `tempPath := tempFile.Name()` that existed only to explain them. `grep -rn "nolint:gosec" internal/` no longer matches any of those four sites. The pre-existing gosec directives elsewhere in the tree (G202/G304/G204 etc., all from before this PR) are untouched and all pass `nolintlint` under v2.12.2 — the lint stage's `0 issues.` covers that. **2. False green claims.** Both fixed. - The commit message is rewritten. It no longer mentions `make check` as a passing gate; it states the verification is `script/cibuild` and explains why (`script/lint` runs whatever `golangci-lint` is on `PATH`, tracked in #78). It also no longer claims gosec suppressions were needed, since they were not. - The `TODO.md` entry now says the work was verified with `script/cibuild`, which exits 0, and explicitly records that `make check` is not a trustworthy gate in this repo today and why. - Bonus: the PR title itself said "make check exits 0". Retitled to "Finish the lint remediation: script/cibuild exits 0 (closes #61)" — still ends with ` (closes #61)`. **3. `TODO.md` dropped a Next Step.** Nothing is silently lost now. The ARCHITECTURE.md item is not restored as a Next Step (it is stale — the working tree is clean and `ARCHITECTURE.md` is committed on `main`); instead it appears in Completed Steps as an explicit "needed no work, and here is why" entry. The Next Step is the stale-branch triage (#71), and the Future Steps item it came from was moved, not deleted — the remaining Future Steps entry (cut v0.1.0) is intact. **4. Inaccurate prealloc comment** at `internal/snapshot/scanner.go`. Rewritten to say what is actually true: a pending file contributes one mapping of each kind per chunk and an empty file contributes none, so the file count is a rough starting capacity for the mapping slices rather than a lower bound — while it is exact for the file and file-ID slices. No code change; capacity only. The commit message also notes this correction, since the superseded `cb25b01` message repeats the old "at least one mapping per file" wording. **Out of scope, confirmed untouched:** - The five `//nolint:revive,nolintlint` package-clause directives in `internal/log` (3 files), `internal/crypto`, `internal/types` — byte-identical to what you reviewed. Naming decision stays with #76. - The 18 `sqlclosecheck` sites and the `prealloc` code change — unchanged. - `.golangci.yml` still `sha256 021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb`; `git diff origin/main -- .golangci.yml Dockerfile Makefile .gitea script` is empty. - #78 not touched. **PR body** updated: the "gosec 4" line in the per-linter breakdown is replaced with an explanation that gosec reports nothing at those sites under the canonical linter and that the earlier directives came from a run of the older local v2.10.1. The obsolete G115/G703 justification bullets are gone; the `sqlclosecheck` correction section is kept as-is. One unrelated thing observed in the lint output, not fixed here: the pinned linter warns that `gomodguard` is deprecated since v2.12.0 in favour of `gomodguard_v2`. That comes from the canonical `.golangci.yml`, which must stay byte-identical, so it needs handling at the config level rather than in this PR.

clawbot added needs-review and removed needs-rework labels 2026-08-09 04:16:22 +02:00

clawbot commented

2026-08-09 04:24:13 +02:00

Re-review of PR #77 at `efb0cea` — VERDICT: PASS

Fresh independent review of the force-pushed head
efb0cea1c2b6d6f7ff2a07a15eaaff6ffc0fd8a1 in a detached worktree. Nothing was
modified, committed, or pushed. I did not assume any finding or "verified clean"
item from the prior review at b960ca3; every claim below was re-measured on
this head.

No blocking findings.

The gate: `script/cibuild`

Run verbatim from the branch worktree:

$ script/cibuild; echo "EXIT=$?"
EXIT=0

That first run resolved entirely from the BuildKit layer cache, so it produced
exit 0 with no visible lint or test output. A cached green is not evidence I am
willing to pass a merge on, so I re-ran the same build uncached
(docker build --no-cache --progress=plain ., i.e. the identical Dockerfile and
digest-pinned images script/cibuild uses) and read every stage:

#16 [lint 7/8] RUN make fmt-check
#16 DONE 6.8s
#17 [lint 8/8] RUN make lint
#17 100.3 0 issues.
#17 DONE 104.6s
...
#24 [builder 8/9] RUN make test
#24 50.18 ok  	sneak.berlin/go/vaultik/internal/blob	1.222s
#24 50.18 ok  	sneak.berlin/go/vaultik/internal/blobgen	1.086s
#24 50.93 ok  	sneak.berlin/go/vaultik/internal/chunker	1.990s
#24 57.31 ok  	sneak.berlin/go/vaultik/internal/cli	1.306s
#24 57.31 ok  	sneak.berlin/go/vaultik/internal/config	1.224s
#24 57.31 ok  	sneak.berlin/go/vaultik/internal/crypto	1.058s
#24 70.62 ok  	sneak.berlin/go/vaultik/internal/database	14.659s
#24 70.62 ok  	sneak.berlin/go/vaultik/internal/globals	1.045s
#24 70.62 ok  	sneak.berlin/go/vaultik/internal/models	1.058s
#24 70.62 ok  	sneak.berlin/go/vaultik/internal/pidlock	1.041s
#24 70.62 ok  	sneak.berlin/go/vaultik/internal/s3	1.641s
#24 70.62 ok  	sneak.berlin/go/vaultik/internal/snapshot	4.878s
#24 70.62 ok  	sneak.berlin/go/vaultik/internal/ui	1.045s
#24 70.62 ok  	sneak.berlin/go/vaultik/internal/vaultik	11.976s
#24 DONE 72.6s
EXIT=0

Confirmed: the lint stage is golangci-lint:v2.12.2-alpine resolved by digest
sha256:91b27804074a0bacea298707f016911e60cf0cdbc6c7bf5ccacb5f0606d18d60 and
reports 0 issues. make fmt-check passes. make test is exactly 14 packages
ok, 4 [no test files], zero (cached) — the tests genuinely executed. The
release go build succeeds. The PR body's verification section is accurate in
every particular.

.gitea/workflows/check.yml runs script/cibuild and nothing else, so this is
the CI build, executed locally against the same pinned images.

Force-push delta `b960ca3` to `efb0cea`

The lower three commits are byte-identical — same SHAs cb25b01, 7a37a66,
047bd7f, not merely equivalent trees. Only b960ca3 was amended.
git diff b960ca3 efb0cea is exactly five hunks and nothing else:

internal/log/log.go — //nolint:gosec and its three-line justification gone.
internal/ui/ui.go — //nolint:gosec and its three-line justification gone.
internal/vaultik/verify.go — both //nolint:gosec // G703 directives gone,
plus the four-line taint comment that existed only to justify them.
internal/snapshot/scanner.go — capacity comment rewritten.
TODO.md — wording.

Nothing was smuggled in. No commit added, none removed, no reordering.

The four dead gosec directives are actually gone

Verified by full-tree enumeration of every //nolint in the branch, diffed
against origin/main, rather than by grepping the four sites. The branch adds
exactly five //nolint directives relative to main and removes none:

internal/log/log.go:3:package log //nolint:revive,nolintlint // stdlib log unused here; see #76
internal/log/module.go:1:package log //nolint:revive,nolintlint // stdlib log unused here; see #76
internal/log/tty_handler.go:1:package log //nolint:revive,nolintlint // stdlib log unused here; see #76
internal/crypto/encryption.go:3:package crypto //nolint:revive,nolintlint // stdlib crypto unused; see #76
internal/types/types.go:5:package types //nolint:revive,nolintlint // rename decision tracked in #76

No //nolint:nolintlint, no broadened suppression, no file-level //nolint, no
//go:build ignore anywhere. internal/ui/ui.go is not in the PR's changed-file
set at all now, so the pre-existing ui.go:243 //nolint:gosec // G115: >=0 is
untouched by construction. .golangci.yml is unmodified, so the directives were
not neutralised at the config level either.

No false green claims survive

PR title: "Finish the lint remediation: script/cibuild exits 0 (closes #61)".
PR body: attributes green to script/cibuild, explains the script/lint
PATH-lookup defect, points at #78, and explicitly warns make check can exit 0
on a tree CI rejects.
All four commit messages: git log cc58583..efb0cea contains no assertion that
make check is green. efb0cea states the gate is script/cibuild and names
#78.
TODO.md: records script/cibuild exits 0 and that make check is not a
trustworthy gate today, with the reason. It also corrects the 2026-08-07 entry
in place, replacing "make check green" with an explicit statement that
make lint was still red and the earlier commit-message claim was wrong.

No repeat of the 7ae470e defect class.

Five revive package-clause directives untouched

git diff b960ca3 efb0cea over the five files yields exactly one nolint line —
the removed gosec one in log.go. All five //nolint:revive,nolintlint
directives are byte-identical to the previously reviewed head.

I probed the flakiness risk implied by their own justification (revive reports
the package-name failure once per package on whichever file it lints first): if
any file in an affected package lacked the directive, lint would be
nondeterministic. Checked every .go file in all three directories. All three
files of internal/log carry it; internal/types has a single file;
internal/crypto has encryption.go (carries it) plus encryption_test.go,
which declares package crypto_test, a different package that revive does not
report against. There is no file that can lose the race unsuppressed, so lint is
deterministic here.

Re-verified from scratch, not carried over

sqlclosecheck, all 18 sites. main had exactly 18 defer CloseRows(rows)
in internal/database (blob_chunks 1, blobs 1, chunk_files 3, chunks 2,
chunks_ext 1, file_chunks 3, files 3, snapshots 4). The branch has 19
rows.Close() sites there: those 18 plus the pre-existing
uploads.go:101, which already used the inline closure idiom on main and only
gained a wsl_v5 blank line. Every replacement is a 1:1 in-place substitution
sitting immediately after the if err != nil { return } guard, so a nil rows
is never dereferenced. Each of the 19 functions has exactly one
rows, err := ...QueryContext(...) and no reassignment anywhere in the package,
so the closure captures the only value it can and is equivalent to the old
defer-time argument evaluation. scanSnapshotRows, scanFileChunks, and
scanChunkFiles contain no Close call, and the full-tree enumeration shows no
other rows.Close() in the package, so no double close is introduced. The
Fatalf-on-close-error path is preserved verbatim at every site. CloseRows has
zero remaining references repo-wide and the now-unused database/sql import was
correctly dropped from internal/database/errors.go; Fatalf itself stays.

prealloc is capacity, not length. Both sites are
make([]T, 0, len(canFlush)) and the test site is make([]byte, 0, len(data)).
Zero length, so no zero-value prepend and no change to any result.

047bd7f is provably whitespace-only. 60 insertions, 0 deletions across 26
files; filtering the unified diff for added or removed lines that are not empty
yields nothing.

Nothing weakened. sha256sum .golangci.yml is
021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb.
git diff origin/main efb0cea -- Dockerfile Makefile .gitea/ script/ .golangci.yml
is empty. No func Test removed, no t.Skip added, no assertion touched — the
diff contains no added or removed line matching any of those.

TODO.md lost nothing. The dropped ARCHITECTURE.md Next Step now appears in
Completed Steps as "needed no work", and that claim is true: ARCHITECTURE.md is
committed on main and the tree is clean. The Future Steps v0.1.0 entry
survives. The stale-branch Future Steps item was moved into Next Step per
TODO.md's own Workflow section and re-pointed at #71, which is where that work
is now tracked.

scanner.go comment is factually correct. A pending file contributes one
mapping of each kind per chunk and a zero-byte file contributes none, so
len(canFlush) is a rough starting capacity for the two mapping slices; and the
comment's further claim that it is exact for allFileIDs and allFiles is true —
the loop appends exactly one element each per canFlush entry.

Mergeable. git merge-base efb0cea origin/main is cc58583, which is
origin/main itself, so this fast-forwards with no conflicts.

Policy. No Claude or Anthropic reference in any of the four commit messages,
the diff, or the PR body. No Co-Authored-By, no session-link trailer, no
trailers at all. Author and committer are sneak on all four commits. Landing
commit title ends with (closes #61), the other three with (refs #61), and
the PR title also ends with (closes #61) so a squash merge lands correctly.
TODO.md is updated in the same commit as the work. No non-inclusive
terminology introduced. No scope creep — the diff contains nothing beyond the
four described changes.

Definition of done for #61. Met. Items 1 through 4 hold (behavior-preserving,
.golangci.yml untouched, reviewable per-linter commits, tests green). Item 5's
literal wording is "make check fully green", which under the #78 tooling defect
is not measurable from a dev shell; it is satisfied in the environment that
matters, since the pinned lint stage runs make lint and make fmt-check and
the builder runs make test, all green. #59 is unblocked by the same result.

Non-blocking nits

Gitea CI has not reported on this head. The single check
check / check (pull_request) (run 87) has been pending / "Waiting to run"
since 04:14:11+02:00 with no runner pickup. This is a runner-availability
issue, not a property of the change — the workflow's only step is
script/cibuild, which I executed uncached against the same digest-pinned
images with exit 0. Worth confirming the status flips green before or shortly
after merge; I would not hold the PR for it.
internal/log/log.go:72 carries an unrelated one-character change versus
main: // Check if stdout is a TTY became // Check if stdout is a TTY..
Residue from collapsing the gosec justification. Harmless noise, but it is a
line in the diff that no lint finding required.
cb25b01's commit message still contains the superseded claim that the
file count is "a safe lower bound since every file contributes at least one
mapping of each kind" — the exact wording efb0cea corrects in the code
comment. efb0cea's message flags the correction explicitly, so the branch
record as a whole is not misleading, and a squash merge takes the PR title
anyway. Since the branch was being rewritten regardless, amending cb25b01
would have left a cleaner history.
#59 will not auto-close. The PR body reads "Closes #61 and, with it, #59";
only the first is a closing keyword. #59 needs closing by hand once this
lands.
The five //nolint:revive,nolintlint directives still shield themselves
from nolintlint — the same mechanism that caught the four dead gosec
directives cannot report a stale revive directive on those lines. Carried
forward unchanged as an accepted trade-off pending #76; noted so it is not
forgotten when that issue is decided.
Observed, correctly not fixed here: the pinned linter warns that
gomodguard is deprecated since v2.12.0 in favour of gomodguard_v2. That
comes from the canonical .golangci.yml, which this PR must not modify.
Belongs at the config level.

The engineering is sound and the rework addressed every blocking finding
precisely, without collateral changes and without weakening anything to get
there. The sqlclosecheck conversion is behavior-identical at all 18 sites, the
prealloc change is inert, the wsl_v5 commit is whitespace, and the record now
claims green only for the gate that actually measures it.

## Re-review of PR #77 at `efb0cea` — VERDICT: PASS Fresh independent review of the force-pushed head `efb0cea1c2b6d6f7ff2a07a15eaaff6ffc0fd8a1` in a detached worktree. Nothing was modified, committed, or pushed. I did not assume any finding or "verified clean" item from the prior review at `b960ca3`; every claim below was re-measured on this head. **No blocking findings.** --- ### The gate: `script/cibuild` Run verbatim from the branch worktree: ``` $ script/cibuild; echo "EXIT=$?" EXIT=0 ``` That first run resolved entirely from the BuildKit layer cache, so it produced exit 0 with no visible lint or test output. A cached green is not evidence I am willing to pass a merge on, so I re-ran the same build uncached (`docker build --no-cache --progress=plain .`, i.e. the identical Dockerfile and digest-pinned images `script/cibuild` uses) and read every stage: ``` #16 [lint 7/8] RUN make fmt-check #16 DONE 6.8s #17 [lint 8/8] RUN make lint #17 100.3 0 issues. #17 DONE 104.6s ... #24 [builder 8/9] RUN make test #24 50.18 ok sneak.berlin/go/vaultik/internal/blob 1.222s #24 50.18 ok sneak.berlin/go/vaultik/internal/blobgen 1.086s #24 50.93 ok sneak.berlin/go/vaultik/internal/chunker 1.990s #24 57.31 ok sneak.berlin/go/vaultik/internal/cli 1.306s #24 57.31 ok sneak.berlin/go/vaultik/internal/config 1.224s #24 57.31 ok sneak.berlin/go/vaultik/internal/crypto 1.058s #24 70.62 ok sneak.berlin/go/vaultik/internal/database 14.659s #24 70.62 ok sneak.berlin/go/vaultik/internal/globals 1.045s #24 70.62 ok sneak.berlin/go/vaultik/internal/models 1.058s #24 70.62 ok sneak.berlin/go/vaultik/internal/pidlock 1.041s #24 70.62 ok sneak.berlin/go/vaultik/internal/s3 1.641s #24 70.62 ok sneak.berlin/go/vaultik/internal/snapshot 4.878s #24 70.62 ok sneak.berlin/go/vaultik/internal/ui 1.045s #24 70.62 ok sneak.berlin/go/vaultik/internal/vaultik 11.976s #24 DONE 72.6s EXIT=0 ``` Confirmed: the lint stage is `golangci-lint:v2.12.2-alpine` resolved by digest `sha256:91b27804074a0bacea298707f016911e60cf0cdbc6c7bf5ccacb5f0606d18d60` and reports `0 issues.` `make fmt-check` passes. `make test` is exactly 14 packages `ok`, 4 `[no test files]`, zero `(cached)` — the tests genuinely executed. The release `go build` succeeds. The PR body's verification section is accurate in every particular. `.gitea/workflows/check.yml` runs `script/cibuild` and nothing else, so this is the CI build, executed locally against the same pinned images. ### Force-push delta `b960ca3` to `efb0cea` The lower three commits are byte-identical — same SHAs `cb25b01`, `7a37a66`, `047bd7f`, not merely equivalent trees. Only `b960ca3` was amended. `git diff b960ca3 efb0cea` is exactly five hunks and nothing else: - `internal/log/log.go` — `//nolint:gosec` and its three-line justification gone. - `internal/ui/ui.go` — `//nolint:gosec` and its three-line justification gone. - `internal/vaultik/verify.go` — both `//nolint:gosec // G703` directives gone, plus the four-line taint comment that existed only to justify them. - `internal/snapshot/scanner.go` — capacity comment rewritten. - `TODO.md` — wording. Nothing was smuggled in. No commit added, none removed, no reordering. ### The four dead gosec directives are actually gone Verified by full-tree enumeration of every `//nolint` in the branch, diffed against `origin/main`, rather than by grepping the four sites. The branch adds exactly five `//nolint` directives relative to `main` and removes none: ``` internal/log/log.go:3:package log //nolint:revive,nolintlint // stdlib log unused here; see #76 internal/log/module.go:1:package log //nolint:revive,nolintlint // stdlib log unused here; see #76 internal/log/tty_handler.go:1:package log //nolint:revive,nolintlint // stdlib log unused here; see #76 internal/crypto/encryption.go:3:package crypto //nolint:revive,nolintlint // stdlib crypto unused; see #76 internal/types/types.go:5:package types //nolint:revive,nolintlint // rename decision tracked in #76 ``` No `//nolint:nolintlint`, no broadened suppression, no file-level `//nolint`, no `//go:build ignore` anywhere. `internal/ui/ui.go` is not in the PR's changed-file set at all now, so the pre-existing `ui.go:243` `//nolint:gosec // G115: >=0` is untouched by construction. `.golangci.yml` is unmodified, so the directives were not neutralised at the config level either. ### No false green claims survive - PR title: "Finish the lint remediation: script/cibuild exits 0 (closes #61)". - PR body: attributes green to `script/cibuild`, explains the `script/lint` PATH-lookup defect, points at #78, and explicitly warns `make check` can exit 0 on a tree CI rejects. - All four commit messages: `git log cc58583..efb0cea` contains no assertion that `make check` is green. `efb0cea` states the gate is `script/cibuild` and names #78. - `TODO.md`: records `script/cibuild` exits 0 and that `make check` is not a trustworthy gate today, with the reason. It also corrects the 2026-08-07 entry in place, replacing "`make check` green" with an explicit statement that `make lint` was still red and the earlier commit-message claim was wrong. No repeat of the `7ae470e` defect class. ### Five revive package-clause directives untouched `git diff b960ca3 efb0cea` over the five files yields exactly one `nolint` line — the removed `gosec` one in `log.go`. All five `//nolint:revive,nolintlint` directives are byte-identical to the previously reviewed head. I probed the flakiness risk implied by their own justification (revive reports the package-name failure once per package on whichever file it lints first): if any file in an affected package lacked the directive, lint would be nondeterministic. Checked every `.go` file in all three directories. All three files of `internal/log` carry it; `internal/types` has a single file; `internal/crypto` has `encryption.go` (carries it) plus `encryption_test.go`, which declares `package crypto_test`, a different package that revive does not report against. There is no file that can lose the race unsuppressed, so lint is deterministic here. ### Re-verified from scratch, not carried over **`sqlclosecheck`, all 18 sites.** `main` had exactly 18 `defer CloseRows(rows)` in `internal/database` (blob_chunks 1, blobs 1, chunk_files 3, chunks 2, chunks_ext 1, file_chunks 3, files 3, snapshots 4). The branch has 19 `rows.Close()` sites there: those 18 plus the pre-existing `uploads.go:101`, which already used the inline closure idiom on `main` and only gained a `wsl_v5` blank line. Every replacement is a 1:1 in-place substitution sitting immediately after the `if err != nil { return }` guard, so a nil `rows` is never dereferenced. Each of the 19 functions has exactly one `rows, err := ...QueryContext(...)` and no reassignment anywhere in the package, so the closure captures the only value it can and is equivalent to the old defer-time argument evaluation. `scanSnapshotRows`, `scanFileChunks`, and `scanChunkFiles` contain no `Close` call, and the full-tree enumeration shows no other `rows.Close()` in the package, so no double close is introduced. The `Fatalf`-on-close-error path is preserved verbatim at every site. `CloseRows` has zero remaining references repo-wide and the now-unused `database/sql` import was correctly dropped from `internal/database/errors.go`; `Fatalf` itself stays. **`prealloc` is capacity, not length.** Both sites are `make([]T, 0, len(canFlush))` and the test site is `make([]byte, 0, len(data))`. Zero length, so no zero-value prepend and no change to any result. **`047bd7f` is provably whitespace-only.** 60 insertions, 0 deletions across 26 files; filtering the unified diff for added or removed lines that are not empty yields nothing. **Nothing weakened.** `sha256sum .golangci.yml` is `021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb`. `git diff origin/main efb0cea -- Dockerfile Makefile .gitea/ script/ .golangci.yml` is empty. No `func Test` removed, no `t.Skip` added, no assertion touched — the diff contains no added or removed line matching any of those. **`TODO.md` lost nothing.** The dropped ARCHITECTURE.md Next Step now appears in Completed Steps as "needed no work", and that claim is true: `ARCHITECTURE.md` is committed on `main` and the tree is clean. The Future Steps `v0.1.0` entry survives. The stale-branch Future Steps item was moved into Next Step per `TODO.md`'s own Workflow section and re-pointed at #71, which is where that work is now tracked. **`scanner.go` comment is factually correct.** A pending file contributes one mapping of each kind per chunk and a zero-byte file contributes none, so `len(canFlush)` is a rough starting capacity for the two mapping slices; and the comment's further claim that it is exact for `allFileIDs` and `allFiles` is true — the loop appends exactly one element each per `canFlush` entry. **Mergeable.** `git merge-base efb0cea origin/main` is `cc58583`, which is `origin/main` itself, so this fast-forwards with no conflicts. **Policy.** No Claude or Anthropic reference in any of the four commit messages, the diff, or the PR body. No `Co-Authored-By`, no session-link trailer, no trailers at all. Author and committer are `sneak` on all four commits. Landing commit title ends with ` (closes #61)`, the other three with ` (refs #61)`, and the PR title also ends with ` (closes #61)` so a squash merge lands correctly. `TODO.md` is updated in the same commit as the work. No non-inclusive terminology introduced. No scope creep — the diff contains nothing beyond the four described changes. **Definition of done for #61.** Met. Items 1 through 4 hold (behavior-preserving, `.golangci.yml` untouched, reviewable per-linter commits, tests green). Item 5's literal wording is "`make check` fully green", which under the #78 tooling defect is not measurable from a dev shell; it is satisfied in the environment that matters, since the pinned lint stage runs `make lint` and `make fmt-check` and the builder runs `make test`, all green. #59 is unblocked by the same result. --- ### Non-blocking nits 1. **Gitea CI has not reported on this head.** The single check `check / check (pull_request)` (run 87) has been `pending` / "Waiting to run" since 04:14:11+02:00 with no runner pickup. This is a runner-availability issue, not a property of the change — the workflow's only step is `script/cibuild`, which I executed uncached against the same digest-pinned images with exit 0. Worth confirming the status flips green before or shortly after merge; I would not hold the PR for it. 2. **`internal/log/log.go:72` carries an unrelated one-character change** versus `main`: `// Check if stdout is a TTY` became `// Check if stdout is a TTY.`. Residue from collapsing the gosec justification. Harmless noise, but it is a line in the diff that no lint finding required. 3. **`cb25b01`'s commit message still contains the superseded claim** that the file count is "a safe lower bound since every file contributes at least one mapping of each kind" — the exact wording `efb0cea` corrects in the code comment. `efb0cea`'s message flags the correction explicitly, so the branch record as a whole is not misleading, and a squash merge takes the PR title anyway. Since the branch was being rewritten regardless, amending `cb25b01` would have left a cleaner history. 4. **#59 will not auto-close.** The PR body reads "Closes #61 and, with it, #59"; only the first is a closing keyword. #59 needs closing by hand once this lands. 5. **The five `//nolint:revive,nolintlint` directives still shield themselves from `nolintlint`** — the same mechanism that caught the four dead `gosec` directives cannot report a stale `revive` directive on those lines. Carried forward unchanged as an accepted trade-off pending #76; noted so it is not forgotten when that issue is decided. 6. **Observed, correctly not fixed here:** the pinned linter warns that `gomodguard` is deprecated since v2.12.0 in favour of `gomodguard_v2`. That comes from the canonical `.golangci.yml`, which this PR must not modify. Belongs at the config level. --- The engineering is sound and the rework addressed every blocking finding precisely, without collateral changes and without weakening anything to get there. The `sqlclosecheck` conversion is behavior-identical at all 18 sites, the `prealloc` change is inert, the `wsl_v5` commit is whitespace, and the record now claims green only for the gate that actually measures it.

clawbot added merge-ready and removed needs-review labels 2026-08-09 04:25:00 +02:00

clawbot merged commit e496aa334b into main

2026-08-09 04:25:11 +02:00

clawbot deleted branch lint-remediation-final

2026-08-09 04:25:12 +02:00

clawbot referenced this pull request

2026-08-09 04:28:26 +02:00

Remediate lint findings surfaced by the standard .golangci.yml #61

clawbot referenced this pull request

2026-08-09 04:40:36 +02:00

Run the linter at the pinned version locally too (closes #78) #79

clawbot referenced this pull request