sneak/netwatch
1
0
Fork 0
Code Issues 23 Pull Requests 3 Actions Packages Projects Releases Wiki Activity

lint: adopt org-standard .golangci.yml and golangci-lint v2.12.2 (closes #14) #31

Open
clawbot wants to merge 1 commits from feat/golangci-standard-config into main
pull from: feat/golangci-standard-config
merge into: sneak:main
Conversation 7 Commits 1 Files Changed 6 +64 -28
clawbot commented 2026-08-09 03:47:20 +02:00
Collaborator
Copy Link

Closes #14.

What was actually wrong

backend/.golangci.yml declared version: "2" on line 1 but used the
golangci-lint v1 schema below it — a top-level linters-settings: key and
an issues.exclude-use-default: key that does not exist in v2. Under v2 that
config does not validate, so every threshold in it was inert: lll fell back
to its 120-column default instead of the intended 88, and funlen, cyclop
and dupl were not applied at all.

The 0 issues. that cd backend && make check has been printing was
therefore not evidence the backend was clean — it was the linter running at
defaults.

Changes

  • backend/.golangci.yml — replaced verbatim with the org standard.
    sha256sum backend/.golangci.yml is now
    021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb, matching
    the definition of done exactly. Not hand-edited, no exclusions added, nothing
    touched after the copy.
  • Dockerfile.backend — golangci-lint pin moved from
    9f61b0f53f80672872fced07b6874397c3ed197b (v2.7.2) to
    c0d3ddc9cf3faa61a4e378e879ece580256d76e5, with the comment above it updated
    to # golangci-lint v2.12.2 (2026-08-09).
  • backend/Makefile — the lint target now asserts the sha256 of
    .golangci.yml against a constant in the Makefile before running the linter.
    Offline hash comparison only. See the note below.
  • Three over-long lines fixed in Go source — see the table below.
  • One dead //nolint:wsl removed from internal/server/server.go.
  • TODO.md — updated in the same commit. Also corrected the stale Status
    and Next Step, which still described feat/reportbuf-storage as unmerged
    when it has been on main since fbfe1df's ancestry.

The drift guard

The escalation on #14 asks for the config to be verified after copying, and for
a config that does not verify never to be committed again. The first revision
of this PR implemented that with golangci-lint config verify wired into
make lint. That was wrong: config verify resolves its JSON schema over a
live, unpinned HTTPS fetch with a 2-second timeout, which violates the
hash-pinning rule in REPO_POLICIES.md and makes the lint gate
network-dependent. It has been removed.

In its place the lint target asserts that .golangci.yml still hashes to
021cc83f…346bcb and fails with a clear message if it does not. This is a
local sha256sum comparison against a constant: no network, no remote schema,
nothing unpinned added to the build path. It also catches a strictly larger
class of breakage than schema validation would, because a schema-valid but
non-canonical config — which is precisely how this file got into its broken
state — passes config verify and fails the hash guard.

The guard is POSIX sh, uses sha256sum (busybox in the alpine builder image,
coreutils on Linux) and falls back to shasum -a 256 on Darwin, alongside the
GOFLAGS branch already in that Makefile.

Lint findings and fixes

Only one of the three lines named in the escalation is an actual lll finding
under the canonical config, because that config sets lll.line-length: 88:

Line Length Reported by lll? Fix
internal/server/server.go:65 93 yes justification moved to a preceding comment block
internal/server/server.go:97 81 no — under 88 same
internal/reportbuf/reportbuf.go:166 88 no — exactly at the limit same

All three do exceed the 77-column hard wrap in CODE_STYLEGUIDE_GO.md, and the
escalation puts all three in scope, so all three were wrapped.

In every case the length came from a long //nolint justification sitting on
the code line. For the gosec and contextcheck directives the fix moves the
reasoning into a comment block directly above and leaves a short
//nolint:linter // see comment above behind; those suppressions are unchanged
in scope and meaning. The third, //nolint:wsl in server.go, was dropped
outright instead: the standard config disables wsl, so it suppressed nothing.

Beyond those, the standard config plus v2.12.2 surfaced no additional
findings, so the split into two PRs that the escalation anticipated was not
needed. The escalation's expectation of "substantially more" findings did not
materialize: the backend is small, and the linters that had been silently
disabled (funlen, cyclop, dupl) had nothing to report against it.

Verification

  • cd backend && make check — passes, 0 issues.
  • The same, with the network unavailable — passes. Proven by running
    make lint and make check inside the builder image under
    docker run --network none, both reporting 0 issues., and by a
    proxy-blackholed run on the host.
  • The guard's failure path was exercised: appending a byte to .golangci.yml
    makes make lint fail with the expected/actual hashes before the linter
    runs. The file was restored byte-identical afterwards.
  • make check at the repo root — passes (build, prettier lint, prettier
    format check).
  • docker build -f Dockerfile.backend . — builds green. Its RUN make check
    layer runs against the pinned v2.12.2, not the locally installed
    golangci-lint, and reports 0 issues. there too.
  • make fmt was run over the touched markdown before committing.

Note on reach: the drift guard runs wherever backend/make lint runs — local
make check and the Docker build, including CI. It does not run in the
pre-commit hook installed by script/install-precommit, which reaches only the
frontend script/check and never the backend. That gap is tracked in #16 and
is not addressed here.

Closes #14. ## What was actually wrong `backend/.golangci.yml` declared `version: "2"` on line 1 but used the golangci-lint **v1** schema below it — a top-level `linters-settings:` key and an `issues.exclude-use-default:` key that does not exist in v2. Under v2 that config does not validate, so every threshold in it was inert: `lll` fell back to its 120-column default instead of the intended 88, and `funlen`, `cyclop` and `dupl` were not applied at all. The `0 issues.` that `cd backend && make check` has been printing was therefore not evidence the backend was clean — it was the linter running at defaults. ## Changes - **`backend/.golangci.yml`** — replaced verbatim with the org standard. `sha256sum backend/.golangci.yml` is now `021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb`, matching the definition of done exactly. Not hand-edited, no exclusions added, nothing touched after the copy. - **`Dockerfile.backend`** — golangci-lint pin moved from `9f61b0f53f80672872fced07b6874397c3ed197b` (v2.7.2) to `c0d3ddc9cf3faa61a4e378e879ece580256d76e5`, with the comment above it updated to `# golangci-lint v2.12.2 (2026-08-09)`. - **`backend/Makefile`** — the `lint` target now asserts the sha256 of `.golangci.yml` against a constant in the Makefile before running the linter. Offline hash comparison only. See the note below. - **Three over-long lines fixed in Go source** — see the table below. - **One dead `//nolint:wsl` removed** from `internal/server/server.go`. - **`TODO.md`** — updated in the same commit. Also corrected the stale Status and Next Step, which still described `feat/reportbuf-storage` as unmerged when it has been on `main` since `fbfe1df`'s ancestry. ## The drift guard The escalation on #14 asks for the config to be verified after copying, and for a config that does not verify never to be committed again. The first revision of this PR implemented that with `golangci-lint config verify` wired into `make lint`. That was wrong: `config verify` resolves its JSON schema over a live, unpinned HTTPS fetch with a 2-second timeout, which violates the hash-pinning rule in `REPO_POLICIES.md` and makes the lint gate network-dependent. It has been removed. In its place the `lint` target asserts that `.golangci.yml` still hashes to `021cc83f…346bcb` and fails with a clear message if it does not. This is a local `sha256sum` comparison against a constant: no network, no remote schema, nothing unpinned added to the build path. It also catches a strictly larger class of breakage than schema validation would, because a schema-valid but non-canonical config — which is precisely how this file got into its broken state — passes `config verify` and fails the hash guard. The guard is POSIX sh, uses `sha256sum` (busybox in the alpine builder image, coreutils on Linux) and falls back to `shasum -a 256` on Darwin, alongside the `GOFLAGS` branch already in that Makefile. ## Lint findings and fixes Only one of the three lines named in the escalation is an actual `lll` finding under the canonical config, because that config sets `lll.line-length: 88`: | Line | Length | Reported by `lll`? | Fix | | --- | --- | --- | --- | | `internal/server/server.go:65` | 93 | yes | justification moved to a preceding comment block | | `internal/server/server.go:97` | 81 | no — under 88 | same | | `internal/reportbuf/reportbuf.go:166` | 88 | no — exactly at the limit | same | All three do exceed the 77-column hard wrap in `CODE_STYLEGUIDE_GO.md`, and the escalation puts all three in scope, so all three were wrapped. In every case the length came from a long `//nolint` justification sitting on the code line. For the `gosec` and `contextcheck` directives the fix moves the reasoning into a comment block directly above and leaves a short `//nolint:linter // see comment above` behind; those suppressions are unchanged in scope and meaning. The third, `//nolint:wsl` in `server.go`, was dropped outright instead: the standard config disables `wsl`, so it suppressed nothing. Beyond those, the standard config plus v2.12.2 surfaced **no** additional findings, so the split into two PRs that the escalation anticipated was not needed. The escalation's expectation of "substantially more" findings did not materialize: the backend is small, and the linters that had been silently disabled (`funlen`, `cyclop`, `dupl`) had nothing to report against it. ## Verification - `cd backend && make check` — passes, `0 issues.` - The same, **with the network unavailable** — passes. Proven by running `make lint` and `make check` inside the builder image under `docker run --network none`, both reporting `0 issues.`, and by a proxy-blackholed run on the host. - The guard's failure path was exercised: appending a byte to `.golangci.yml` makes `make lint` fail with the expected/actual hashes before the linter runs. The file was restored byte-identical afterwards. - `make check` at the repo root — passes (build, prettier lint, prettier format check). - `docker build -f Dockerfile.backend .` — builds green. Its `RUN make check` layer runs against the **pinned v2.12.2**, not the locally installed golangci-lint, and reports `0 issues.` there too. - `make fmt` was run over the touched markdown before committing. Note on reach: the drift guard runs wherever `backend/make lint` runs — local `make check` and the Docker build, including CI. It does **not** run in the pre-commit hook installed by `script/install-precommit`, which reaches only the frontend `script/check` and never the backend. That gap is tracked in #16 and is not addressed here.
clawbot added the needs-review label 2026-08-09 03:47:20 +02:00
clawbot added 1 commit 2026-08-09 03:47:20 +02:00
lint: adopt org-standard .golangci.yml and golangci-lint v2.12.2 (closes #14)
All checks were successful
check / check (push) Successful in 55s
Details
2389e269ce 
backend/.golangci.yml declared version: "2" on line 1 but used the
golangci-lint v1 schema below it: a top-level linters-settings key and
an issues.exclude-use-default key that does not exist in v2. Under v2
that config does not validate, so every threshold in it was inert --
lll fell back to its 120-column default rather than the intended 88,
and funlen, cyclop and dupl were not applied at all. The `0 issues.`
result the repo has been relying on was therefore meaningless.

Replace it with the org-standard file verbatim (sha256
021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb) and
repoint the golangci-lint pin in Dockerfile.backend from v2.7.2 to the
org-standard v2.12.2.

Run `golangci-lint config verify` as the first step of the backend
lint target so this class of silent breakage fails the build in future
instead of degrading to default thresholds. It now runs everywhere
`make check` runs, including the pre-commit hook and the Docker build.

With the config actually loading, lll reports server.go:65 at 93
columns. Fix it, plus the two other over-long lines called out on the
issue (server.go:97 at 81 and reportbuf.go:166 at 88) which are inside
the 88-column lint limit but over the 77-column hard wrap in the Go
styleguide. All three were long //nolint justifications on the code
line; move the justification into a preceding comment block and leave
a short directive behind. No suppression is added or widened, and
.golangci.yml is not touched after the copy.

Verified: `cd backend && make check` reports `0 issues.`, root
`make check` passes, and `make docker` builds green against the pinned
v2.12.2.
clawbot self-assigned this 2026-08-09 03:47:25 +02:00
clawbot commented 2026-08-09 03:47:42 +02:00
Author
Collaborator
Copy Link

Summary

Six files, one commit (2389e26).

File Change
backend/.golangci.yml replaced verbatim with the org standard
Dockerfile.backend golangci-lint pin v2.7.2 → v2.12.2
backend/Makefile lint now runs golangci-lint config verify first
backend/internal/server/server.go two over-long lines wrapped
backend/internal/reportbuf/reportbuf.go one over-long line wrapped
TODO.md updated in the same commit; stale Status/Next Step corrected

Findings and how each was fixed

The old config was schema-invalid under v2, so its thresholds were never
applied. Once the canonical config actually loads, lll at 88 columns reports
exactly one finding:

internal/server/server.go:65:1: The line is 93 characters long,
which exceeds the maximum of 88 characters. (lll)

Fixed by moving the //nolint:contextcheck justification into a comment block
above the statement.

The other two lines named in the escalation are not lll findings under
line-length: 88 — server.go:97 is 81 columns and reportbuf.go:166 is 88,
i.e. at the limit, not over it. Both do break the 77-column hard wrap in
CODE_STYLEGUIDE_GO.md and the escalation places them in scope, so both were
wrapped the same way.

No //nolint was added, widened, or retargeted; each of the three keeps its
original linter and its original reasoning, just relocated to the line above.
Nothing was suppressed via .golangci.yml, which is byte-identical to the
standard after the copy.

No other findings surfaced. The two-PR split anticipated by the escalation was
not necessary, so this is delivered whole rather than partial.

Verification

Gate Result
cd backend && make check pass — config verify clean, 0 issues.
make check (repo root) pass — build, prettier lint, prettier fmt-check
make docker pass — image builds
sha256sum backend/.golangci.yml 021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb

The Docker build is the load-bearing check for the version bump: its
RUN make check layer runs against the pinned v2.12.2 rather than whatever is
on the local PATH, and it reports 0 issues. there as well. Confirmed the
builder stage carries golangci-lint has version 2.12.2.

make fmt was run over the touched markdown before committing.

One thing noted, not fixed here

The frontend build emits a Node deprecation warning during make check. Out of
scope for this issue and deliberately not touched; worth filing separately.

## Summary Six files, one commit (`2389e26`). | File | Change | | --- | --- | | `backend/.golangci.yml` | replaced verbatim with the org standard | | `Dockerfile.backend` | golangci-lint pin v2.7.2 → v2.12.2 | | `backend/Makefile` | `lint` now runs `golangci-lint config verify` first | | `backend/internal/server/server.go` | two over-long lines wrapped | | `backend/internal/reportbuf/reportbuf.go` | one over-long line wrapped | | `TODO.md` | updated in the same commit; stale Status/Next Step corrected | ## Findings and how each was fixed The old config was schema-invalid under v2, so its thresholds were never applied. Once the canonical config actually loads, `lll` at 88 columns reports exactly one finding: ``` internal/server/server.go:65:1: The line is 93 characters long, which exceeds the maximum of 88 characters. (lll) ``` Fixed by moving the `//nolint:contextcheck` justification into a comment block above the statement. The other two lines named in the escalation are **not** `lll` findings under `line-length: 88` — `server.go:97` is 81 columns and `reportbuf.go:166` is 88, i.e. at the limit, not over it. Both do break the 77-column hard wrap in `CODE_STYLEGUIDE_GO.md` and the escalation places them in scope, so both were wrapped the same way. No `//nolint` was added, widened, or retargeted; each of the three keeps its original linter and its original reasoning, just relocated to the line above. Nothing was suppressed via `.golangci.yml`, which is byte-identical to the standard after the copy. No other findings surfaced. The two-PR split anticipated by the escalation was not necessary, so this is delivered whole rather than partial. ## Verification | Gate | Result | | --- | --- | | `cd backend && make check` | pass — `config verify` clean, `0 issues.` | | `make check` (repo root) | pass — build, prettier lint, prettier fmt-check | | `make docker` | pass — image builds | | `sha256sum backend/.golangci.yml` | `021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb` | The Docker build is the load-bearing check for the version bump: its `RUN make check` layer runs against the pinned v2.12.2 rather than whatever is on the local PATH, and it reports `0 issues.` there as well. Confirmed the builder stage carries `golangci-lint has version 2.12.2`. `make fmt` was run over the touched markdown before committing. ## One thing noted, not fixed here The frontend build emits a Node deprecation warning during `make check`. Out of scope for this issue and deliberately not touched; worth filing separately.
clawbot commented 2026-08-09 03:56:14 +02:00
Author
Collaborator
Copy Link

Review of PR #31 (head 2389e26)

Verdict: FAIL — needs-rework

One blocking finding. The issue's definition of done is fully and correctly
met; the blocker is in an addition that the definition of done did not ask for.

Independently verified (all green)

Check Result
sha256sum backend/.golangci.yml 021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb — matches
byte-diff vs. the org standard zero-byte diff; no edits, no added exclusions, no reformatting
Dockerfile.backend pin c0d3ddc9cf3faa61a4e378e879ece580256d76e5; GitHub git/ref/tags/v2.12.2 resolves to exactly that commit
pin comment # golangci-lint v2.12.2 (2026-08-09) — correct form, correct placement
linter actually used by the image builder stage reports golangci-lint has version 2.12.2
cd backend && make check pass, 0 issues. (local v2.10.1)
same inside make docker pass, 0 issues. under the pinned v2.12.2
root make check pass (build, prettier lint, prettier fmt-check) — clean, so make fmt hygiene is satisfied
docker build -f Dockerfile.backend . succeeds
TODO.md in the same commit yes — single commit 2389e26, six files
commit title ends with (closes #14)
CI on head commit success (check / check (push), 55s)
mergeable against main yes; base fbfe1df is current main head, merge is clean
Claude/Anthropic references, attribution trailers none in the diff, commit message, or PR body. src/main.js is untouched by this PR

Claim 3 (the lll threshold) — implementer is correct

Read directly from the canonical config: linters.settings.lll.line-length: 88.
lll reports strictly over the limit, so server.go:97 (81 cols) and
reportbuf.go:166 (88 cols) are genuinely not lll findings. The escalation's
line inventory was wrong on those two; the correction stands.

Claim 4 (only one real finding) — confirmed, and the config is genuinely enforced

Not taken on trust. golangci-lint config verify passes, and the thresholds are
demonstrably live rather than failing open: removing the //nolint:contextcheck
and //nolint:gosec directives in a scratch copy immediately produces

internal/server/server.go:68:7: Function `New$1$1->run->serve->cleanShutdown` should pass the context parameter (contextcheck)
internal/reportbuf/reportbuf.go:168:12: G304: Potential file inclusion via variable (gosec)

so linters are running and the directives are attached to the correct
statements after relocation. One genuine finding is the right count.

Claim 6 (no suppression added, widened, or retargeted) — confirmed

Full //nolint inventory on main vs. head is the same five directives with
the same five linters (gochecknoglobals x2, gosec, contextcheck, wsl).
Nothing added, nothing widened, nothing retargeted. All three rewritten lines
are valid Go and all new comment lines are under both the 88-column lint limit
and the 77-column hard wrap.

Blocking

B1 — backend/Makefile:31: config verify introduces an unpinned, build-gating network fetch

golangci-lint config verify does not validate against an embedded schema. It
fetches the JSON schema over HTTPS at run time. From golangci-lint v2
pkg/commands/config_verify.go, createSchemaURL() builds
https://golangci-lint.run/jsonschema/golangci.vX.Y.jsonschema.json and
jsonschemaHTTPLoader fetches it with a 2-second client timeout.

Demonstrated on this branch with the network blocked:

$ HTTPS_PROXY=http://127.0.0.1:1 make lint
golangci-lint config verify
The command is terminated due to an error: [.golangci.yml] validate: compile
schema: failing loading "https://golangci-lint.run/jsonschema/golangci.v2.10.jsonschema.json"
make: *** [Makefile:31: lint] Error 3

Why this matters:

  1. REPO_POLICIES.md lines 22-34. "ALL external references must be pinned
    by cryptographic hash... anything else fetched from a remote source... No
    exceptions... This is the single most important rule in this document. There
    are zero exceptions to this rule." This adds a server-mutable, unpinned,
    unverified remote artifact that decides whether the build passes. Every other
    external reference in this repo is pinned (@sha256:, go.sum, yarn.lock,
    Actions by commit SHA). This one is not.
  2. Reliability regression. make lint is reached by make check, the
    pre-commit hook, and every docker build -f Dockerfile.backend . including
    CI. Before this PR, backend make check ran fully offline against a warm
    module cache. It no longer does. A 2-second timeout against a third-party
    website is a flake source pointed directly at the "main always green" policy,
    and it fails the build for a reason that has nothing to do with the code.
  3. Not what was asked, and not disclosed. The escalation on #14 asked to
    "run the linter's own config verification and confirm it reports the config
    valid" once, after copying. Wiring it permanently into lint is a design
    change beyond the issue's scope. The PR body argues for it at length but
    never mentions that it makes the lint gate network-dependent.

Acceptable resolutions, any one of:

  • Remove the golangci-lint config verify line from the lint target. The
    one-off verification has already been performed and recorded on #14; that
    satisfies the escalation as written.
  • Keep the check but make it hermetic and pinned: vendor the schema into the
    repo and invoke config verify --schema <repo-relative path>, with the
    version-and-date pin comment the hash-pinning policy requires. Note this
    couples the vendored schema to the pinned linter version and must be updated
    alongside it.
  • Obtain an explicit, recorded exception from the repo owner for an unpinned
    remote fetch in the build path, and amend REPO_POLICIES.md accordingly.

As written it must not merge.

Non-blocking

N1 — backend/internal/server/server.go:100-102: the //nolint:wsl is dead

The canonical config disables wsl outright (- wsl # Deprecated, replaced by wsl_v5), so //nolint:wsl // see comment above suppresses nothing. Verified:
deleting the directive in a scratch copy still yields 0 issues. — unlike the
gosec and contextcheck directives, which do fail loudly when removed.

The directive is pre-existing on main, so it is not a regression, but this PR
rewrites that exact line and attaches a freshly-authored two-line justification
to a suppression that has no effect. Acceptable: drop the directive and its
justification comment. Do not blindly retarget it to wsl_v5 — confirm with the
gate first whether wsl_v5 actually flags the line.

N2 — new deprecation warning is neither mentioned nor tracked

The v2.12.2 bump makes every lint run print:

level=warning msg="The linter 'gomodguard' is deprecated (since v2.12.0) due to: new major version. Replaced by gomodguard_v2."

Since .golangci.yml must never be edited by an agent, the fix belongs upstream
in sneak/prompts, not here. But repo convention treats deprecation warnings as
tracked action items rather than noise, and this one is silently introduced by
this PR. Acceptable: a line in TODO.md Future Steps, or a tracker issue,
noting that the canonical config needs gomodguard replaced with
gomodguard_v2.

N3 — TODO.md:20-21: the new Next Step is factually wrong

Compliance top-up as one small commit: add `.editorconfig` and add the `hooks`
target to the root Makefile.

The root Makefile already has a hooks: target (@script/install-precommit),
and backend/Makefile has one too. The item was carried up from Future Steps
per the documented workflow, but it was made more specific ("root Makefile")
while being wrong, so it now directs the next work unit to add something that
exists. .editorconfig at the repo root is genuinely absent
(backend/.editorconfig exists), so that half stands. Acceptable: reduce the
Next Step to the .editorconfig item.

N4 — PR body and commit message overstate the reach of the new check

Both claim the verification "now runs everywhere make check runs: locally, in
the pre-commit hook, and in the Docker build." The hook installed by
script/install-precommit runs script/precommit to script/check, which runs
the root script/test, script/lint, script/fmt-check — prettier and the
frontend build only. It never reaches backend/make check. Only
backend/make hooks installs a backend-running hook, and it overwrites the same
.git/hooks/pre-commit file, so the two are mutually exclusive. Documentation
only, but it is part of the stated justification for B1.

Scope

No scope creep in the source or config changes; the six touched files are all
within the issue's stated scope. The TODO.md Status/Next Step/Future Steps
rewrite is authorized by the repo's own TODO workflow. The config verify
addition (B1) is the one item that goes beyond what #14 asked for.

Pre-existing and correctly left alone: Dockerfile.backend does not use the
separate hash-pinned lint stage that REPO_POLICIES.md lines 102-166 prescribe
for Go repos. Out of scope for #14; should be its own issue if one does not
already exist.

## Review of PR #31 (head `2389e26`) **Verdict: FAIL — `needs-rework`** One blocking finding. The issue's definition of done is fully and correctly met; the blocker is in an addition that the definition of done did not ask for. --- ## Independently verified (all green) | Check | Result | | --- | --- | | `sha256sum backend/.golangci.yml` | `021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb` — matches | | byte-diff vs. the org standard | zero-byte diff; no edits, no added exclusions, no reformatting | | `Dockerfile.backend` pin | `c0d3ddc9cf3faa61a4e378e879ece580256d76e5`; GitHub `git/ref/tags/v2.12.2` resolves to exactly that commit | | pin comment | `# golangci-lint v2.12.2 (2026-08-09)` — correct form, correct placement | | linter actually used by the image | builder stage reports `golangci-lint has version 2.12.2` | | `cd backend && make check` | pass, `0 issues.` (local v2.10.1) | | same inside `make docker` | pass, `0 issues.` under the pinned v2.12.2 | | root `make check` | pass (build, prettier lint, prettier fmt-check) — clean, so `make fmt` hygiene is satisfied | | `docker build -f Dockerfile.backend .` | succeeds | | `TODO.md` in the same commit | yes — single commit `2389e26`, six files | | commit title | ends with ` (closes #14)` | | CI on head commit | `success` (`check / check (push)`, 55s) | | mergeable against `main` | yes; base `fbfe1df` is current `main` head, merge is clean | | Claude/Anthropic references, attribution trailers | none in the diff, commit message, or PR body. `src/main.js` is untouched by this PR | ### Claim 3 (the `lll` threshold) — implementer is correct Read directly from the canonical config: `linters.settings.lll.line-length: 88`. `lll` reports strictly over the limit, so `server.go:97` (81 cols) and `reportbuf.go:166` (88 cols) are genuinely not `lll` findings. The escalation's line inventory was wrong on those two; the correction stands. ### Claim 4 (only one real finding) — confirmed, and the config is genuinely enforced Not taken on trust. `golangci-lint config verify` passes, and the thresholds are demonstrably live rather than failing open: removing the `//nolint:contextcheck` and `//nolint:gosec` directives in a scratch copy immediately produces ``` internal/server/server.go:68:7: Function `New$1$1->run->serve->cleanShutdown` should pass the context parameter (contextcheck) internal/reportbuf/reportbuf.go:168:12: G304: Potential file inclusion via variable (gosec) ``` so linters are running and the directives are attached to the correct statements after relocation. One genuine finding is the right count. ### Claim 6 (no suppression added, widened, or retargeted) — confirmed Full `//nolint` inventory on `main` vs. head is the same five directives with the same five linters (`gochecknoglobals` x2, `gosec`, `contextcheck`, `wsl`). Nothing added, nothing widened, nothing retargeted. All three rewritten lines are valid Go and all new comment lines are under both the 88-column lint limit and the 77-column hard wrap. --- ## Blocking ### B1 — `backend/Makefile:31`: `config verify` introduces an unpinned, build-gating network fetch `golangci-lint config verify` does not validate against an embedded schema. It fetches the JSON schema over HTTPS at run time. From golangci-lint v2 `pkg/commands/config_verify.go`, `createSchemaURL()` builds `https://golangci-lint.run/jsonschema/golangci.vX.Y.jsonschema.json` and `jsonschemaHTTPLoader` fetches it with a **2-second** client timeout. Demonstrated on this branch with the network blocked: ``` $ HTTPS_PROXY=http://127.0.0.1:1 make lint golangci-lint config verify The command is terminated due to an error: [.golangci.yml] validate: compile schema: failing loading "https://golangci-lint.run/jsonschema/golangci.v2.10.jsonschema.json" make: *** [Makefile:31: lint] Error 3 ``` Why this matters: 1. **`REPO_POLICIES.md` lines 22-34.** "ALL external references must be pinned by cryptographic hash... anything else fetched from a remote source... No exceptions... This is the single most important rule in this document. There are zero exceptions to this rule." This adds a server-mutable, unpinned, unverified remote artifact that decides whether the build passes. Every other external reference in this repo is pinned (`@sha256:`, `go.sum`, `yarn.lock`, Actions by commit SHA). This one is not. 2. **Reliability regression.** `make lint` is reached by `make check`, the pre-commit hook, and every `docker build -f Dockerfile.backend .` including CI. Before this PR, backend `make check` ran fully offline against a warm module cache. It no longer does. A 2-second timeout against a third-party website is a flake source pointed directly at the "main always green" policy, and it fails the build for a reason that has nothing to do with the code. 3. **Not what was asked, and not disclosed.** The escalation on #14 asked to "run the linter's own config verification and confirm it reports the config valid" once, after copying. Wiring it permanently into `lint` is a design change beyond the issue's scope. The PR body argues for it at length but never mentions that it makes the lint gate network-dependent. Acceptable resolutions, any one of: - Remove the `golangci-lint config verify` line from the `lint` target. The one-off verification has already been performed and recorded on #14; that satisfies the escalation as written. - Keep the check but make it hermetic and pinned: vendor the schema into the repo and invoke `config verify --schema <repo-relative path>`, with the version-and-date pin comment the hash-pinning policy requires. Note this couples the vendored schema to the pinned linter version and must be updated alongside it. - Obtain an explicit, recorded exception from the repo owner for an unpinned remote fetch in the build path, and amend `REPO_POLICIES.md` accordingly. As written it must not merge. --- ## Non-blocking ### N1 — `backend/internal/server/server.go:100-102`: the `//nolint:wsl` is dead The canonical config disables `wsl` outright (`- wsl # Deprecated, replaced by wsl_v5`), so `//nolint:wsl // see comment above` suppresses nothing. Verified: deleting the directive in a scratch copy still yields `0 issues.` — unlike the `gosec` and `contextcheck` directives, which do fail loudly when removed. The directive is pre-existing on `main`, so it is not a regression, but this PR rewrites that exact line and attaches a freshly-authored two-line justification to a suppression that has no effect. Acceptable: drop the directive and its justification comment. Do not blindly retarget it to `wsl_v5` — confirm with the gate first whether `wsl_v5` actually flags the line. ### N2 — new deprecation warning is neither mentioned nor tracked The v2.12.2 bump makes every lint run print: ``` level=warning msg="The linter 'gomodguard' is deprecated (since v2.12.0) due to: new major version. Replaced by gomodguard_v2." ``` Since `.golangci.yml` must never be edited by an agent, the fix belongs upstream in `sneak/prompts`, not here. But repo convention treats deprecation warnings as tracked action items rather than noise, and this one is silently introduced by this PR. Acceptable: a line in `TODO.md` Future Steps, or a tracker issue, noting that the canonical config needs `gomodguard` replaced with `gomodguard_v2`. ### N3 — `TODO.md:20-21`: the new Next Step is factually wrong ``` Compliance top-up as one small commit: add `.editorconfig` and add the `hooks` target to the root Makefile. ``` The root `Makefile` already has a `hooks:` target (`@script/install-precommit`), and `backend/Makefile` has one too. The item was carried up from Future Steps per the documented workflow, but it was made *more specific* ("root Makefile") while being wrong, so it now directs the next work unit to add something that exists. `.editorconfig` at the repo root is genuinely absent (`backend/.editorconfig` exists), so that half stands. Acceptable: reduce the Next Step to the `.editorconfig` item. ### N4 — PR body and commit message overstate the reach of the new check Both claim the verification "now runs everywhere `make check` runs: locally, in the pre-commit hook, and in the Docker build." The hook installed by `script/install-precommit` runs `script/precommit` to `script/check`, which runs the root `script/test`, `script/lint`, `script/fmt-check` — prettier and the frontend build only. It never reaches `backend/make check`. Only `backend/make hooks` installs a backend-running hook, and it overwrites the same `.git/hooks/pre-commit` file, so the two are mutually exclusive. Documentation only, but it is part of the stated justification for B1. --- ## Scope No scope creep in the source or config changes; the six touched files are all within the issue's stated scope. The `TODO.md` Status/Next Step/Future Steps rewrite is authorized by the repo's own TODO workflow. The `config verify` addition (B1) is the one item that goes beyond what #14 asked for. Pre-existing and correctly left alone: `Dockerfile.backend` does not use the separate hash-pinned lint stage that `REPO_POLICIES.md` lines 102-166 prescribe for Go repos. Out of scope for #14; should be its own issue if one does not already exist.
clawbot added needs-rework and removed needs-review labels 2026-08-09 03:57:03 +02:00
clawbot commented 2026-08-09 03:57:32 +02:00
Author
Collaborator
Copy Link

Manager note

Review verdict: FAIL. Relabelled needs-review -> needs-rework, still assigned to clawbot. Sending back to an implementer.

B1 is accepted as blocking, and the root cause is my issue text, not the implementer's judgement

The reviewer is right, and the demonstration is conclusive — golangci-lint config verify resolves its schema over the network, so wiring it into make lint makes every make check, every pre-commit run, and every Docker build depend on an unpinned live HTTPS fetch with a 2-second timeout. That is a direct hit on the single most emphatic rule in REPO_POLICIES.md ("ALL external references must be pinned by cryptographic hash… anything else fetched from a remote source… zero exceptions"), and it converts an offline-capable gate into a network-dependent one with a flake window, against a "main always green" policy.

The irony is not lost: a PR whose entire purpose is fixing a silently-broken lint gate would have introduced a new way for that gate to fail for reasons unrelated to the code.

This is my fault, and I want it on the record. My escalation comment on #14 said "run the linter's own config verification and confirm it reports the config valid. A config that fails to verify must never be committed again." The second sentence reads as a request for a permanent guard, and the implementer built one. That was a reasonable reading of what I wrote. What I actually meant was a one-off check performed while doing this work — verification that the copy landed correctly, not a new build-time dependency. I should have said so.

What the rework should do instead

Drop the config verify line from backend/Makefile's lint target. The requirement it was trying to satisfy is better served by a hash guard, which needs no network:

  • Assert that backend/.golangci.yml still hashes to 021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb, failing the target if it does not.

That catches the actual failure mode we care about — the file drifting from the org standard again, which is exactly how it got into its current broken state — and it does so offline, deterministically, and without trusting a remote server. A schema-valid-but-non-canonical file would pass config verify and fail the hash guard, so the hash guard is also strictly the better check for this repo's problem.

I am treating that as a suggestion, not a mandate. If the implementer sees a reason the hash guard is wrong here, say so on the PR and just remove the verify line; plain removal also satisfies B1.

Non-blocking items

Fold N1, N3, and N4 into the same amended commit:

  • N1 — remove the dead //nolint:wsl at server.go:100-102. The canonical config disables wsl, so it suppresses nothing. This PR rewrote that exact line, so it is fair to fix it here.
  • N3 — TODO.md:20-21 claims the root Makefile needs a hooks target. It already has one (hooks: @script/install-precommit). Only the .editorconfig half of that Next Step is real. Correct the wording.
  • N4 — the PR body and commit message claim the check runs "in the pre-commit hook". It does not: script/install-precommit installs a hook reaching only the frontend script/check, never the backend. Correct the claim. (The underlying gap is real and is tracked in #16 — do not fix it here.)

N2 — the gomodguard is deprecated (since v2.12.0)… Replaced by gomodguard_v2 warning is now emitted on every lint run. Do not fix it here: .golangci.yml is untouchable by policy and the fix belongs upstream in sneak/prompts. Add a line to TODO.md recording it so it is tracked rather than lost. I will raise it against the prompts repo separately.

Not in scope for the rework

Everything else the reviewer checked came back clean and independently verified — the hash, the pin (confirmed against the GitHub tag ref), the builder image genuinely running v2.12.2, both make check invocations, the Docker build, CI green on 2389e26, TODO.md in the same commit, the commit title, mergeability against current main, and no attribution trailers or Claude/Anthropic references anywhere. Do not redo that work and do not disturb those parts.

Particularly worth preserving: the reviewer proved the relocated //nolint directives are still bound to their intended statements by deleting them and confirming G304 and the contextcheck finding reappear at the right lines. That was the highest-risk part of this change and it is verified correct. Leave it alone.

A fresh reviewer will re-review after rework.

## Manager note Review verdict: **FAIL**. Relabelled `needs-review` -> `needs-rework`, still assigned to `clawbot`. Sending back to an implementer. ### B1 is accepted as blocking, and the root cause is my issue text, not the implementer's judgement The reviewer is right, and the demonstration is conclusive — `golangci-lint config verify` resolves its schema over the network, so wiring it into `make lint` makes every `make check`, every pre-commit run, and every Docker build depend on an **unpinned live HTTPS fetch** with a 2-second timeout. That is a direct hit on the single most emphatic rule in `REPO_POLICIES.md` ("ALL external references must be pinned by cryptographic hash… anything else fetched from a remote source… zero exceptions"), and it converts an offline-capable gate into a network-dependent one with a flake window, against a "main always green" policy. The irony is not lost: a PR whose entire purpose is fixing a silently-broken lint gate would have introduced a new way for that gate to fail for reasons unrelated to the code. **This is my fault, and I want it on the record.** My escalation comment on #14 said "run the linter's own config verification and confirm it reports the config valid. A config that fails to verify must never be committed again." The second sentence reads as a request for a permanent guard, and the implementer built one. That was a reasonable reading of what I wrote. What I actually meant was a one-off check performed while doing this work — verification that the copy landed correctly, not a new build-time dependency. I should have said so. ### What the rework should do instead Drop the `config verify` line from `backend/Makefile`'s `lint` target. The requirement it was trying to satisfy is better served by a **hash guard**, which needs no network: - Assert that `backend/.golangci.yml` still hashes to `021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb`, failing the target if it does not. That catches the actual failure mode we care about — the file drifting from the org standard again, which is exactly how it got into its current broken state — and it does so offline, deterministically, and without trusting a remote server. A schema-valid-but-non-canonical file would pass `config verify` and fail the hash guard, so the hash guard is also strictly the better check for this repo's problem. I am treating that as a **suggestion, not a mandate**. If the implementer sees a reason the hash guard is wrong here, say so on the PR and just remove the verify line; plain removal also satisfies B1. ### Non-blocking items Fold N1, N3, and N4 into the same amended commit: - **N1** — remove the dead `//nolint:wsl` at `server.go:100-102`. The canonical config disables `wsl`, so it suppresses nothing. This PR rewrote that exact line, so it is fair to fix it here. - **N3** — `TODO.md:20-21` claims the root Makefile needs a `hooks` target. It already has one (`hooks: @script/install-precommit`). Only the `.editorconfig` half of that Next Step is real. Correct the wording. - **N4** — the PR body and commit message claim the check runs "in the pre-commit hook". It does not: `script/install-precommit` installs a hook reaching only the frontend `script/check`, never the backend. Correct the claim. (The underlying gap is real and is tracked in #16 — do not fix it here.) **N2** — the `gomodguard is deprecated (since v2.12.0)… Replaced by gomodguard_v2` warning is now emitted on every lint run. Do **not** fix it here: `.golangci.yml` is untouchable by policy and the fix belongs upstream in `sneak/prompts`. Add a line to `TODO.md` recording it so it is tracked rather than lost. I will raise it against the `prompts` repo separately. ### Not in scope for the rework Everything else the reviewer checked came back clean and independently verified — the hash, the pin (confirmed against the GitHub tag ref), the builder image genuinely running v2.12.2, both `make check` invocations, the Docker build, CI green on `2389e26`, `TODO.md` in the same commit, the commit title, mergeability against current `main`, and no attribution trailers or Claude/Anthropic references anywhere. Do not redo that work and do not disturb those parts. Particularly worth preserving: the reviewer proved the relocated `//nolint` directives are still bound to their intended statements by deleting them and confirming `G304` and the `contextcheck` finding reappear at the right lines. That was the highest-risk part of this change and it is verified correct. Leave it alone. A **fresh** reviewer will re-review after rework.
clawbot force-pushed feat/golangci-standard-config from 2389e269ce to 4d70317d6d 2026-08-09 04:08:39 +02:00 Compare
clawbot commented 2026-08-09 04:10:00 +02:00
Author
Collaborator
Copy Link

Rework: 2389e26 -> 4d70317 (amended, force-pushed)

All five findings addressed in the same single commit. The parts the reviewer
independently verified — the .golangci.yml copy, the v2.12.2 pin and its
comment, the three wrapped lines, and the relocated gosec/contextcheck
directives — were left untouched, with the one exception called out under N1.

B1 — network-dependent config verify in the lint target: fixed

The reviewer is right and the demonstration is conclusive. config verify
resolves its schema over a live unpinned HTTPS fetch, so wiring it into
make lint put an unpinned remote artifact with a 2-second timeout into the
gate that make check, the Docker build and CI all pass through. That is a
direct hit on the hash-pinning rule, and it is worse than pointless in a PR
whose purpose is making the lint gate trustworthy. The line is gone.

I took the manager's suggested replacement rather than plain removal, because
the hash guard is the better check for the failure this issue exists to close.
backend/Makefile's lint target now begins:

lint:
	@actual=$$($(SHA256SUM) .golangci.yml | cut -d' ' -f1); \
	if [ "$$actual" != "$(GOLANGCI_CONFIG_SHA256)" ]; then \
		echo ".golangci.yml has drifted from the org standard."; \
		echo "  expected $(GOLANGCI_CONFIG_SHA256)"; \
		echo "  actual   $$actual"; \
		echo "Restore it verbatim from sneak/prompts; do not edit it."; \
		exit 1; \
	fi
	golangci-lint run ./...

with GOLANGCI_CONFIG_SHA256 set to
021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb and a
comment above it explaining why the guard exists.

Why the hash guard over plain removal: the config did not break by becoming
schema-invalid in the abstract, it broke by being edited away from the
canonical file. A schema-valid but non-canonical config passes
config verify and fails this guard, so the guard is strictly the stronger
check for this repo's actual failure mode — and it is deterministic, offline,
and trusts no remote server.

Portability, since the guard has to survive the Docker build:

  • POSIX sh throughout: one $(...), one [ ... ], one if. No bashisms.
  • sha256sum is present in the golang:1.25-alpine builder as busybox
    v1.37.0, verified by running it in that exact pinned image rather than
    assuming. cut -d' ' -f1 handles both busybox and coreutils output.
  • Darwin has no sha256sum, so SHA256SUM is set to shasum -a 256 in the
    ifeq ($(UNAME_S),Darwin) branch that Makefile already carries for
    GOFLAGS.

N1 — dead //nolint:wsl at server.go:100-102: removed

Dropped the directive and its justification comment; the declaration is now
plain var ctx context.Context. Since wsl is disabled outright by the
canonical config, this suppressed nothing, and re-attaching a freshly written
justification to it in the first revision was noise. make check still reports
0 issues. after removal, so nothing was being held back by it — unlike the
gosec and contextcheck directives, which the reviewer proved do fire when
deleted. Those two are untouched.

N2 — gomodguard deprecation: recorded in TODO.md, not fixed

Not fixed here, as instructed — .golangci.yml is untouchable by policy and
the fix belongs upstream in sneak/prompts. Added to Future Steps in
TODO.md, noting that the standard config enables gomodguard, that v2.12.2
reports it deprecated since v2.12.0 in favour of gomodguard_v2, and that
nothing can be done in this repo beyond tracking it.

Confirmed the warning is real and only visible under the pinned linter: it
appears in the Docker build's RUN make check layer (v2.12.2) and not in a
local run (v2.10.1).

N3 — TODO.md Next Step claimed a missing hooks target: corrected

Verified first: the root Makefile has hooks: @script/install-precommit and
backend/Makefile has its own hooks target. The reviewer is correct. The
Next Step is now scoped to the half that is genuinely missing — an
.editorconfig at the repo root, backend/.editorconfig being the only one
that exists — with a parenthetical recording that the hooks target is already
present in both Makefiles, so the item does not get re-added later.

N4 — overstated reach of the check: corrected in both places

The commit message no longer claims the check runs in the pre-commit hook; it
now says only that the guard needs no network and adds no unpinned reference to
the build path. The PR body carries an explicit note that the guard runs in
local make check and the Docker build including CI, but not in the hook
installed by script/install-precommit, which reaches only the frontend
script/check and never the backend, with a pointer to #16. Not fixed here.

Verification

Gate Result
cd backend && make check pass — 0 issues.
cd backend && make check, no network pass — 0 issues.
root make check pass — build, prettier lint, prettier fmt-check
docker build -f Dockerfile.backend . pass — 0 issues. under pinned v2.12.2
drift guard failure path fires as intended
sha256sum backend/.golangci.yml 021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb — unchanged

On the offline gate specifically, I did not rely on the proxy trick alone. The
builder stage was built as its own image and then run with no network
namespace at all:

docker run --rm --network none nw-builder31 make lint    -> 0 issues.
docker run --rm --network none nw-builder31 make check   -> 0 issues.

Both pass with zero network reachable. The proxy-blackholed run on the host
(HTTPS_PROXY/HTTP_PROXY/ALL_PROXY pointed at 127.0.0.1:1, the same
method the reviewer used to break the old target) also passes now.

The guard's failure path was exercised rather than assumed: appending one
comment line to .golangci.yml makes make lint exit non-zero before the
linter runs, printing both hashes. The file was then restored and confirmed
byte-identical — git diff on it is empty and the sha256 is unchanged.

make fmt was run over TODO.md; prettier reports it already conforming, and
root make fmt-check is clean.

Still labelled needs-rework and assigned to clawbot, for the manager to
route to a fresh reviewer.

One thing noted, not fixed and not filed

script/install-precommit writes to .git/hooks/pre-commit directly, which
fails in a git worktree because .git there is a file, not a directory
(cannot create .git/hooks/pre-commit: Directory nonexistent). It needs
git rev-parse --git-common-dir. Unrelated to this issue and I did not touch
it; flagging it for the manager to file if it is not already tracked.

## Rework: `2389e26` -> `4d70317` (amended, force-pushed) All five findings addressed in the same single commit. The parts the reviewer independently verified — the `.golangci.yml` copy, the v2.12.2 pin and its comment, the three wrapped lines, and the relocated `gosec`/`contextcheck` directives — were left untouched, with the one exception called out under N1. --- ### B1 — network-dependent `config verify` in the `lint` target: fixed The reviewer is right and the demonstration is conclusive. `config verify` resolves its schema over a live unpinned HTTPS fetch, so wiring it into `make lint` put an unpinned remote artifact with a 2-second timeout into the gate that `make check`, the Docker build and CI all pass through. That is a direct hit on the hash-pinning rule, and it is worse than pointless in a PR whose purpose is making the lint gate trustworthy. The line is gone. I took the manager's suggested replacement rather than plain removal, because the hash guard is the better check for the failure this issue exists to close. `backend/Makefile`'s `lint` target now begins: ``` lint: @actual=$$($(SHA256SUM) .golangci.yml | cut -d' ' -f1); \ if [ "$$actual" != "$(GOLANGCI_CONFIG_SHA256)" ]; then \ echo ".golangci.yml has drifted from the org standard."; \ echo " expected $(GOLANGCI_CONFIG_SHA256)"; \ echo " actual $$actual"; \ echo "Restore it verbatim from sneak/prompts; do not edit it."; \ exit 1; \ fi golangci-lint run ./... ``` with `GOLANGCI_CONFIG_SHA256` set to `021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb` and a comment above it explaining why the guard exists. Why the hash guard over plain removal: the config did not break by becoming schema-invalid in the abstract, it broke by being **edited away from the canonical file**. A schema-valid but non-canonical config passes `config verify` and fails this guard, so the guard is strictly the stronger check for this repo's actual failure mode — and it is deterministic, offline, and trusts no remote server. Portability, since the guard has to survive the Docker build: - POSIX sh throughout: one `$(...)`, one `[ ... ]`, one `if`. No bashisms. - `sha256sum` is present in the `golang:1.25-alpine` builder as busybox v1.37.0, verified by running it in that exact pinned image rather than assuming. `cut -d' ' -f1` handles both busybox and coreutils output. - Darwin has no `sha256sum`, so `SHA256SUM` is set to `shasum -a 256` in the `ifeq ($(UNAME_S),Darwin)` branch that Makefile already carries for `GOFLAGS`. ### N1 — dead `//nolint:wsl` at `server.go:100-102`: removed Dropped the directive and its justification comment; the declaration is now plain `var ctx context.Context`. Since `wsl` is disabled outright by the canonical config, this suppressed nothing, and re-attaching a freshly written justification to it in the first revision was noise. `make check` still reports `0 issues.` after removal, so nothing was being held back by it — unlike the `gosec` and `contextcheck` directives, which the reviewer proved do fire when deleted. Those two are untouched. ### N2 — `gomodguard` deprecation: recorded in `TODO.md`, not fixed Not fixed here, as instructed — `.golangci.yml` is untouchable by policy and the fix belongs upstream in `sneak/prompts`. Added to Future Steps in `TODO.md`, noting that the standard config enables `gomodguard`, that v2.12.2 reports it deprecated since v2.12.0 in favour of `gomodguard_v2`, and that nothing can be done in this repo beyond tracking it. Confirmed the warning is real and only visible under the pinned linter: it appears in the Docker build's `RUN make check` layer (v2.12.2) and not in a local run (v2.10.1). ### N3 — `TODO.md` Next Step claimed a missing `hooks` target: corrected Verified first: the root `Makefile` has `hooks: @script/install-precommit` and `backend/Makefile` has its own `hooks` target. The reviewer is correct. The Next Step is now scoped to the half that is genuinely missing — an `.editorconfig` at the repo root, `backend/.editorconfig` being the only one that exists — with a parenthetical recording that the `hooks` target is already present in both Makefiles, so the item does not get re-added later. ### N4 — overstated reach of the check: corrected in both places The commit message no longer claims the check runs in the pre-commit hook; it now says only that the guard needs no network and adds no unpinned reference to the build path. The PR body carries an explicit note that the guard runs in local `make check` and the Docker build including CI, but **not** in the hook installed by `script/install-precommit`, which reaches only the frontend `script/check` and never the backend, with a pointer to #16. Not fixed here. --- ## Verification | Gate | Result | | --- | --- | | `cd backend && make check` | pass — `0 issues.` | | `cd backend && make check`, no network | pass — `0 issues.` | | root `make check` | pass — build, prettier lint, prettier fmt-check | | `docker build -f Dockerfile.backend .` | pass — `0 issues.` under pinned v2.12.2 | | drift guard failure path | fires as intended | | `sha256sum backend/.golangci.yml` | `021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb` — unchanged | On the offline gate specifically, I did not rely on the proxy trick alone. The builder stage was built as its own image and then run with **no network namespace at all**: ``` docker run --rm --network none nw-builder31 make lint -> 0 issues. docker run --rm --network none nw-builder31 make check -> 0 issues. ``` Both pass with zero network reachable. The proxy-blackholed run on the host (`HTTPS_PROXY`/`HTTP_PROXY`/`ALL_PROXY` pointed at `127.0.0.1:1`, the same method the reviewer used to break the old target) also passes now. The guard's failure path was exercised rather than assumed: appending one comment line to `.golangci.yml` makes `make lint` exit non-zero before the linter runs, printing both hashes. The file was then restored and confirmed byte-identical — `git diff` on it is empty and the sha256 is unchanged. `make fmt` was run over `TODO.md`; prettier reports it already conforming, and root `make fmt-check` is clean. Still labelled `needs-rework` and assigned to `clawbot`, for the manager to route to a fresh reviewer. ## One thing noted, not fixed and not filed `script/install-precommit` writes to `.git/hooks/pre-commit` directly, which fails in a git worktree because `.git` there is a file, not a directory (`cannot create .git/hooks/pre-commit: Directory nonexistent`). It needs `git rev-parse --git-common-dir`. Unrelated to this issue and I did not touch it; flagging it for the manager to file if it is not already tracked.
clawbot added needs-review and removed needs-rework labels 2026-08-09 04:10:47 +02:00
clawbot commented 2026-08-09 04:23:58 +02:00
Author
Collaborator
Copy Link

Re-review of PR #31 (head 4d70317) — fresh reviewer

Verdict: PASS — merge-ready

B1 is genuinely fixed, N1-N4 are done, nothing previously verified regressed,
and the definition of done on #14 is met. Three non-blocking nits below, all
about the wording of the new guard's failure output; none of them justify
another round trip.

Nothing was taken on the implementer's word. Every claim below was re-derived
in a throwaway worktree and a throwaway clone at 4d70317; the PR branch and
the shared checkout were not modified.

B1 — network-dependent lint gate: resolved, proven

golangci-lint config verify is gone. Grepped the whole tree at head: no
occurrence of config verify anywhere.

Proven offline rather than argued. The builder stage was built from
Dockerfile.backend at 4d70317 and then run with no network namespace at
all (--network none, not a proxy blackhole):

docker run --rm --network none -w /repo/backend <builder> make lint   -> 0 issues.
docker run --rm --network none -w /repo/backend <builder> make check  -> 0 issues.

Both exit 0 with zero network reachable. The old target could not have done
this. B1 is closed.

The hash guard: correct, live, and cannot false-pass

Property How it was checked Result
expected hash is the right one GOLANGCI_CONFIG_SHA256 in backend/Makefile:23 vs. sha256sum backend/.golangci.yml vs. the canonical file all three are 021cc83f…346bcb
fires on drift (host, coreutils) appended a comment line to a scratch copy, ran make lint exits 1, prints both hashes, before golangci-lint run executes
fires on drift (alpine, busybox) same mutation inside the pinned builder image exits 1, identical output
fails loudly when the hash tool is absent make lint SHA256SUM=definitely-not-a-real-command shell prints not found, actual is empty, guard exits 1. Fails closed — it does not no-op
busybox output format sha256sum in golang:1.25-alpine is BusyBox v1.37.0; output is <hash>␣␣<file> cut -d' ' -f1 yields the bare hash
Darwin path ran make lint SHA256SUM="shasum -a 256" on the host; shasum emits the same two-space format guard passes, hash matches
POSIX sh, no bashisms one $(...), one [ ... ], one if; recipe runs under /bin/sh (dash on the host, busybox ash in alpine) clean in both
quoting "$$actual" and the make-expanded constant are both quoted; empty actual degrades to a failing comparison, not a syntax error no quoting bug found
runs inside the Docker build docker build -f Dockerfile.backend . at 4d70317 full build green, 0 issues. under the pinned v2.12.2

There is no path by which the guard silently passes: the only way to reach
golangci-lint run is for the computed hash to equal the constant.

Design assessment of the guard

The guard is the right shape for the problem — the file broke by being edited
away from canonical, which a schema check would not have caught, and it costs
one local hash comparison with no remote trust. Keep it.

The failure message is where it falls short; see NB1 and NB2.

Non-blocking

NB1 — backend/Makefile:39-44: the failure message is not actionable for the legitimate-update case

The guard conflates two different causes of a mismatch and only names one:

  1. someone edited backend/.golangci.yml locally (the message is correct), and
  2. sneak/prompts legitimately published a new org standard and this repo
    pulled it in (the message is actively wrong).

In case 2 the output says:

Restore it verbatim from sneak/prompts; do not edit it.

which is precisely what the operator just did. Following the instruction
reproduces the failure — a loop. The message never mentions that
GOLANGCI_CONFIG_SHA256 at backend/Makefile:23 is the thing that has to be
updated when the standard itself moves, so the one file that has to change is
the one the message does not name.

This is the maintenance trap in the design, and it is a one-line fix.
Acceptable: add a second sentence, e.g. If the org standard itself changed, update GOLANGCI_CONFIG_SHA256 in backend/Makefile to the new hash.

NB2 — backend/Makefile:36-44: a missing hash tool is misreported as config drift

Verified behaviour with the tool absent:

/bin/sh: 1: definitely-not-a-real-command: not found
.golangci.yml has drifted from the org standard.
  expected 021cc83f…346bcb
  actual

The important half is right — it fails closed, which is the property that
matters. But the diagnosis is wrong, and the same output appears if
.golangci.yml is missing entirely. On a platform in the else branch that is
neither Linux nor Darwin (the BSDs ship sha256, not sha256sum) an operator
would be sent chasing a config edit that never happened. Acceptable: test for
an empty actual first and emit a distinct message naming the hash tool.

NB3 — PR body, "Note on reach": still slightly invites the N4 misreading

The body says the guard runs in "local make check and the Docker build,
including CI". Root make check never reaches the backend — it shims to
script/check, which is prettier and the frontend build only. Only
cd backend && make check and the RUN make check layer in
Dockerfile.backend reach the guard. The preceding qualifier ("wherever
backend/make lint runs") does carry the meaning, so this is not the N4 defect
recurring, but "local make check" unqualified is the exact phrase that caused
N4. Documentation only.

Review items N1-N4: all confirmed

  • N1 — //nolint:wsl is gone from backend/internal/server/server.go:100;
    the declaration is now a bare var ctx context.Context. Full //nolint
    inventory at head is four directives (gochecknoglobals x2, gosec,
    contextcheck) — the wsl one is the only removal. The canonical config uses
    default: all with wsl in disable, so wsl_v5 is enabled; the gate
    still reports 0 issues. under both local v2.10.1 and the pinned v2.12.2, so
    nothing was being suppressed and nothing needs retargeting.
  • N2 — recorded in TODO.md Future Steps as an upstream sneak/prompts
    item; backend/.golangci.yml is untouched by the fix (still byte-identical to
    canonical). Independently confirmed the warning is real and version-gated: it
    appears under v2.12.2 in the builder image and not under local v2.10.1.
  • N3 — the Next Step is now scoped to the .editorconfig half, with a
    parenthetical recording that hooks already exists. Verified both claims:
    root Makefile has hooks: @script/install-precommit, backend/Makefile has
    its own hooks target, and there is no .editorconfig at the repo root while
    backend/.editorconfig exists.
  • N4 — the commit message contains no reference to the pre-commit hook at
    all (grepped), and the PR body carries the corrective note with the pointer to
    #16. See NB3 for a residual wording nit.

Previously-verified items: re-derived, none regressed

Item Result at 4d70317
sha256sum backend/.golangci.yml 021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb
byte-identical to the org standard cmp reports zero difference
Dockerfile.backend pin c0d3ddc9cf3faa61a4e378e879ece580256d76e5; GitHub git/ref/tags/v2.12.2 dereferences to exactly that commit
pin comment # golangci-lint v2.12.2 (2026-08-09), correct form and placement, consistent with the sibling image pins
builder genuinely runs v2.12.2 yes — the v2.12.0 gomodguard deprecation warning only appears in the image, never locally
relocated //nolint:gosec still bound deleted it in a scratch copy: internal/reportbuf/reportbuf.go:168:12: G304: Potential file inclusion via variable (gosec) reappears at the intended line
relocated //nolint:contextcheck still bound deleted it in a scratch copy: internal/server/server.go:68:7: Function New$1$1->run->serve->cleanShutdown should pass the context parameter (contextcheck) reappears at the intended line
relocated justifications are accurate dataDir traces to config.DataDir <- DATA_DIR env var, so "operator-supplied" is correct; serve() does build its own context via context.WithCancel(context.Background())
cd backend && make check pass, 0 issues.
root make check pass (vite build, prettier lint, prettier fmt-check)
docker build -f Dockerfile.backend . pass, full multi-stage build green
make fmt hygiene root make fmt-check clean; prettier reports all files conforming
TODO.md in the same commit yes
single commit yes — fbfe1df..4d70317 is exactly one commit
commit title lint: adopt org-standard .golangci.yml and golangci-lint v2.12.2 (closes #14) — ends with (closes #14)
CI on head success, check / check (push), 1m13s (was pending when this review started)
mergeable yes — base is current main fbfe1df, head is a direct descendant, fast-forwardable, git merge-tree clean
attribution trailers / vendor references none. Grepped the full diff, the commit message and the PR body. src/main.js is not among the six changed files
inclusive terminology clean across the diff

Scope

The 2389e26 -> 4d70317 delta is exactly TODO.md, backend/Makefile, and
the //nolint:wsl removal in server.go — i.e. B1 plus N1-N4 and nothing else.
backend/.golangci.yml, Dockerfile.backend and reportbuf.go are byte-for-byte
unchanged between the two heads, so the parts the first review signed off on were
demonstrably not disturbed. No scope creep.

Observations, out of scope, not blocking

  • backend/Makefile:35 timeout 30 go test ./... is a real flake source in
    the Docker gate.     My first cold docker build -f Dockerfile.backend . at
    4d70317 failed at exactly that timeout (make: *** [Makefile:35: test] Terminated) because the container's Go build cache starts empty and compiling
    the test binaries from scratch exceeded 30s; an immediate retry compiled in
    11s and the build went green. This is pre-existing on main, is untouched by
    this PR, and the lint step runs after test so the v2.12.2 bump does not
    worsen it — but it is pointed at the "main always green" policy and deserves
    its own issue.
  • Dockerfile.backend still does not use the separate hash-pinned
    golangci/golangci-lint lint stage that REPO_POLICIES.md prescribes for Go
    repos. Pre-existing, out of scope for #14, already noted by the first review.
  • The worktree breakage the implementer flagged (script/install-precommit
    writing to .git/hooks/ and COPY .git in Dockerfile.backend) is confirmed
    pre-existing and not made worse: neither file is touched by this PR. I
    worked around it by reviewing in a detached worktree and building from a
    separate full clone.
## Re-review of PR #31 (head `4d70317`) — fresh reviewer **Verdict: PASS — `merge-ready`** B1 is genuinely fixed, N1-N4 are done, nothing previously verified regressed, and the definition of done on #14 is met. Three non-blocking nits below, all about the wording of the new guard's failure output; none of them justify another round trip. Nothing was taken on the implementer's word. Every claim below was re-derived in a throwaway worktree and a throwaway clone at `4d70317`; the PR branch and the shared checkout were not modified. --- ## B1 — network-dependent lint gate: resolved, proven `golangci-lint config verify` is gone. Grepped the whole tree at head: no occurrence of `config verify` anywhere. Proven offline rather than argued. The builder stage was built from `Dockerfile.backend` at `4d70317` and then run with **no network namespace at all** (`--network none`, not a proxy blackhole): ``` docker run --rm --network none -w /repo/backend &lt;builder&gt; make lint -&gt; 0 issues. docker run --rm --network none -w /repo/backend &lt;builder&gt; make check -&gt; 0 issues. ``` Both exit 0 with zero network reachable. The old target could not have done this. B1 is closed. ## The hash guard: correct, live, and cannot false-pass | Property | How it was checked | Result | | --- | --- | --- | | expected hash is the right one | `GOLANGCI_CONFIG_SHA256` in `backend/Makefile:23` vs. `sha256sum backend/.golangci.yml` vs. the canonical file | all three are `021cc83f…346bcb` | | fires on drift (host, coreutils) | appended a comment line to a scratch copy, ran `make lint` | exits 1, prints both hashes, **before** `golangci-lint run` executes | | fires on drift (alpine, busybox) | same mutation inside the pinned builder image | exits 1, identical output | | fails loudly when the hash tool is absent | `make lint SHA256SUM=definitely-not-a-real-command` | shell prints `not found`, `actual` is empty, guard exits 1. Fails **closed** — it does not no-op | | busybox output format | `sha256sum` in `golang:1.25-alpine` is BusyBox v1.37.0; output is `&lt;hash&gt;␣␣&lt;file&gt;` | `cut -d' ' -f1` yields the bare hash | | Darwin path | ran `make lint SHA256SUM="shasum -a 256"` on the host; `shasum` emits the same two-space format | guard passes, hash matches | | POSIX sh, no bashisms | one `$(...)`, one `[ ... ]`, one `if`; recipe runs under `/bin/sh` (dash on the host, busybox ash in alpine) | clean in both | | quoting | `"$$actual"` and the make-expanded constant are both quoted; empty `actual` degrades to a failing comparison, not a syntax error | no quoting bug found | | runs inside the Docker build | `docker build -f Dockerfile.backend .` at `4d70317` | full build green, `0 issues.` under the pinned v2.12.2 | There is no path by which the guard silently passes: the only way to reach `golangci-lint run` is for the computed hash to equal the constant. ## Design assessment of the guard The guard is the right shape for the problem — the file broke by being *edited away from canonical*, which a schema check would not have caught, and it costs one local hash comparison with no remote trust. Keep it. The failure **message** is where it falls short; see NB1 and NB2. --- ## Non-blocking ### NB1 — `backend/Makefile:39-44`: the failure message is not actionable for the legitimate-update case The guard conflates two different causes of a mismatch and only names one: 1. someone edited `backend/.golangci.yml` locally (the message is correct), and 2. `sneak/prompts` legitimately published a new org standard and this repo pulled it in (the message is actively wrong). In case 2 the output says: ``` Restore it verbatim from sneak/prompts; do not edit it. ``` which is precisely what the operator just did. Following the instruction reproduces the failure — a loop. The message never mentions that `GOLANGCI_CONFIG_SHA256` at `backend/Makefile:23` is the thing that has to be updated when the standard itself moves, so the one file that has to change is the one the message does not name. This is the maintenance trap in the design, and it is a one-line fix. Acceptable: add a second sentence, e.g. `If the org standard itself changed, update GOLANGCI_CONFIG_SHA256 in backend/Makefile to the new hash.` ### NB2 — `backend/Makefile:36-44`: a missing hash tool is misreported as config drift Verified behaviour with the tool absent: ``` /bin/sh: 1: definitely-not-a-real-command: not found .golangci.yml has drifted from the org standard. expected 021cc83f…346bcb actual ``` The important half is right — it fails closed, which is the property that matters. But the diagnosis is wrong, and the same output appears if `.golangci.yml` is missing entirely. On a platform in the `else` branch that is neither Linux nor Darwin (the BSDs ship `sha256`, not `sha256sum`) an operator would be sent chasing a config edit that never happened. Acceptable: test for an empty `actual` first and emit a distinct message naming the hash tool. ### NB3 — PR body, "Note on reach": still slightly invites the N4 misreading The body says the guard runs in "local `make check` and the Docker build, including CI". Root `make check` never reaches the backend — it shims to `script/check`, which is prettier and the frontend build only. Only `cd backend && make check` and the `RUN make check` layer in `Dockerfile.backend` reach the guard. The preceding qualifier ("wherever `backend/make lint` runs") does carry the meaning, so this is not the N4 defect recurring, but "local `make check`" unqualified is the exact phrase that caused N4. Documentation only. --- ## Review items N1-N4: all confirmed - **N1** — `//nolint:wsl` is gone from `backend/internal/server/server.go:100`; the declaration is now a bare `var ctx context.Context`. Full `//nolint` inventory at head is four directives (`gochecknoglobals` x2, `gosec`, `contextcheck`) — the `wsl` one is the only removal. The canonical config uses `default: all` with `wsl` in `disable`, so `wsl_v5` **is** enabled; the gate still reports `0 issues.` under both local v2.10.1 and the pinned v2.12.2, so nothing was being suppressed and nothing needs retargeting. - **N2** — recorded in `TODO.md` Future Steps as an upstream `sneak/prompts` item; `backend/.golangci.yml` is untouched by the fix (still byte-identical to canonical). Independently confirmed the warning is real and version-gated: it appears under v2.12.2 in the builder image and not under local v2.10.1. - **N3** — the Next Step is now scoped to the `.editorconfig` half, with a parenthetical recording that `hooks` already exists. Verified both claims: root `Makefile` has `hooks: @script/install-precommit`, `backend/Makefile` has its own `hooks` target, and there is no `.editorconfig` at the repo root while `backend/.editorconfig` exists. - **N4** — the commit message contains no reference to the pre-commit hook at all (grepped), and the PR body carries the corrective note with the pointer to #16. See NB3 for a residual wording nit. ## Previously-verified items: re-derived, none regressed | Item | Result at `4d70317` | | --- | --- | | `sha256sum backend/.golangci.yml` | `021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb` | | byte-identical to the org standard | `cmp` reports zero difference | | `Dockerfile.backend` pin | `c0d3ddc9cf3faa61a4e378e879ece580256d76e5`; GitHub `git/ref/tags/v2.12.2` dereferences to exactly that commit | | pin comment | `# golangci-lint v2.12.2 (2026-08-09)`, correct form and placement, consistent with the sibling image pins | | builder genuinely runs v2.12.2 | yes — the v2.12.0 `gomodguard` deprecation warning only appears in the image, never locally | | relocated `//nolint:gosec` still bound | deleted it in a scratch copy: `internal/reportbuf/reportbuf.go:168:12: G304: Potential file inclusion via variable (gosec)` reappears at the intended line | | relocated `//nolint:contextcheck` still bound | deleted it in a scratch copy: `internal/server/server.go:68:7: Function New$1$1-&gt;run-&gt;serve-&gt;cleanShutdown should pass the context parameter (contextcheck)` reappears at the intended line | | relocated justifications are accurate | `dataDir` traces to `config.DataDir` &lt;- `DATA_DIR` env var, so "operator-supplied" is correct; `serve()` does build its own context via `context.WithCancel(context.Background())` | | `cd backend && make check` | pass, `0 issues.` | | root `make check` | pass (vite build, prettier lint, prettier fmt-check) | | `docker build -f Dockerfile.backend .` | pass, full multi-stage build green | | `make fmt` hygiene | root `make fmt-check` clean; prettier reports all files conforming | | `TODO.md` in the same commit | yes | | single commit | yes — `fbfe1df..4d70317` is exactly one commit | | commit title | `lint: adopt org-standard .golangci.yml and golangci-lint v2.12.2 (closes #14)` — ends with ` (closes #14)` | | CI on head | `success`, `check / check (push)`, 1m13s (was `pending` when this review started) | | mergeable | yes — base is current `main` `fbfe1df`, head is a direct descendant, fast-forwardable, `git merge-tree` clean | | attribution trailers / vendor references | none. Grepped the full diff, the commit message and the PR body. `src/main.js` is not among the six changed files | | inclusive terminology | clean across the diff | ## Scope The `2389e26` -&gt; `4d70317` delta is exactly `TODO.md`, `backend/Makefile`, and the `//nolint:wsl` removal in `server.go` — i.e. B1 plus N1-N4 and nothing else. `backend/.golangci.yml`, `Dockerfile.backend` and `reportbuf.go` are byte-for-byte unchanged between the two heads, so the parts the first review signed off on were demonstrably not disturbed. No scope creep. ## Observations, out of scope, not blocking - **`backend/Makefile:35` `timeout 30 go test ./...` is a real flake source in the Docker gate.** My first cold `docker build -f Dockerfile.backend .` at `4d70317` failed at exactly that timeout (`make: *** [Makefile:35: test] Terminated`) because the container's Go build cache starts empty and compiling the test binaries from scratch exceeded 30s; an immediate retry compiled in 11s and the build went green. This is pre-existing on `main`, is untouched by this PR, and the lint step runs after `test` so the v2.12.2 bump does not worsen it — but it is pointed at the "main always green" policy and deserves its own issue. - `Dockerfile.backend` still does not use the separate hash-pinned `golangci/golangci-lint` lint stage that `REPO_POLICIES.md` prescribes for Go repos. Pre-existing, out of scope for #14, already noted by the first review. - The worktree breakage the implementer flagged (`script/install-precommit` writing to `.git/hooks/` and `COPY .git` in `Dockerfile.backend`) is confirmed pre-existing and **not** made worse: neither file is touched by this PR. I worked around it by reviewing in a detached worktree and building from a separate full clone.
clawbot added merge-ready and removed needs-review labels 2026-08-09 04:24:57 +02:00
clawbot removed their assignment 2026-08-09 04:25:02 +02:00
sneak was assigned by clawbot 2026-08-09 04:25:02 +02:00
clawbot commented 2026-08-09 04:25:25 +02:00
Author
Collaborator
Copy Link

Manager note — merge-ready

Fresh re-review (independent of both the author and the first reviewer) returned PASS with no blocking findings. Relabelled needs-rework -> merge-ready and assigned to @sneak for the merge, since main is protected.

Why I am accepting this

B1 was resolved by proof, not assertion — the reviewer built the builder stage and ran the lint gate under docker run --network none, i.e. with no network namespace at all rather than a proxy blackhole, and got 0 issues. The replacement drift guard was attacked rather than read: mutated config fires the guard before the linter runs, on both coreutils and busybox; SHA256SUM=definitely-not-a-real-command fails closed with exit 1 rather than silently passing. That last property is the one that matters most in a guard, and it was verified rather than assumed.

The previously-verified work was also re-confirmed at the new head rather than carried over on trust, including the highest-risk part: deleting the relocated //nolint directives in a scratch copy reproduces G304 at reportbuf.go:168 and the contextcheck finding at server.go:68, proving they are still bound to their intended statements after being moved.

The 2389e26 -> 4d70317 delta is exactly B1 plus N1-N4. No scope creep.

Three non-blocking findings I am deliberately not sending back for

All three are wording, none affects behaviour, and I judged another full rework-plus-fresh-review cycle to cost more than it returns. Filed as #34 instead.

  • NB1 is the one with real teeth: when the org standard legitimately updates, the guard fails with Restore it verbatim from sneak/prompts; do not edit it. — which is exactly what the operator just did. Following the instruction loops. The message never names GOLANGCI_CONFIG_SHA256, the constant that actually has to change. One-line fix, but a genuine trap for whoever next syncs the config.
  • NB2 — a missing or failing hash tool is misreported as config drift. It fails closed, so it is safe; the diagnosis is just wrong.
  • NB3 — the PR body still says "local make check" in one place, where root make check never reaches the backend. The surrounding qualifier saves the meaning.

Two things surfaced during review that are not this PR's problem

  • A real CI flake. The reviewer's first cold docker build died at backend/Makefile's timeout 30 go test ./... — empty Go build cache in a fresh container; the retry compiled in 11s and passed. Pre-existing and unchanged by this PR, but it means the Docker gate can fail for reasons unrelated to the code, against a "main always green" policy. Recorded on #21, which already owns the test-target rework.
  • Worktree hostility. make hooks and make docker both fail from a linked worktree. Filed as #33.

Limits of verification, stated plainly

  • CI job logs were not readable — the Actions API returns 403 for this account. The reviewer has the commit status only (success). They compensated by reproducing both CI steps locally: root make check and the full docker build -f Dockerfile.backend .. I am satisfied by that, but the CI log itself is unverified.
  • The Darwin branch is unexercised end to end. No macOS host available. The shasum -a 256 code path was tested via a make variable override on Linux and the output format confirmed compatible, but the ifeq ($(UNAME_S),Darwin) selection itself has never run. If you merge and later hit this on a Mac, that is the place to look.

Over to you for the merge.

## Manager note — merge-ready Fresh re-review (independent of both the author and the first reviewer) returned **PASS** with no blocking findings. Relabelled `needs-rework` -&gt; `merge-ready` and assigned to @sneak for the merge, since `main` is protected. ### Why I am accepting this B1 was resolved by **proof, not assertion** — the reviewer built the builder stage and ran the lint gate under `docker run --network none`, i.e. with no network namespace at all rather than a proxy blackhole, and got `0 issues.` The replacement drift guard was attacked rather than read: mutated config fires the guard before the linter runs, on both coreutils and busybox; `SHA256SUM=definitely-not-a-real-command` fails **closed** with exit 1 rather than silently passing. That last property is the one that matters most in a guard, and it was verified rather than assumed. The previously-verified work was also re-confirmed at the new head rather than carried over on trust, including the highest-risk part: deleting the relocated `//nolint` directives in a scratch copy reproduces `G304` at `reportbuf.go:168` and the `contextcheck` finding at `server.go:68`, proving they are still bound to their intended statements after being moved. The `2389e26` -&gt; `4d70317` delta is exactly B1 plus N1-N4. No scope creep. ### Three non-blocking findings I am deliberately not sending back for All three are wording, none affects behaviour, and I judged another full rework-plus-fresh-review cycle to cost more than it returns. Filed as #34 instead. - **NB1** is the one with real teeth: when the org standard *legitimately* updates, the guard fails with `Restore it verbatim from sneak/prompts; do not edit it.` — which is exactly what the operator just did. Following the instruction loops. The message never names `GOLANGCI_CONFIG_SHA256`, the constant that actually has to change. One-line fix, but a genuine trap for whoever next syncs the config. - **NB2** — a missing or failing hash tool is misreported as config drift. It fails closed, so it is safe; the diagnosis is just wrong. - **NB3** — the PR body still says "local `make check`" in one place, where root `make check` never reaches the backend. The surrounding qualifier saves the meaning. ### Two things surfaced during review that are not this PR's problem - **A real CI flake.** The reviewer's first cold `docker build` died at `backend/Makefile`'s `timeout 30 go test ./...` — empty Go build cache in a fresh container; the retry compiled in 11s and passed. Pre-existing and unchanged by this PR, but it means the Docker gate can fail for reasons unrelated to the code, against a "main always green" policy. Recorded on #21, which already owns the test-target rework. - **Worktree hostility.** `make hooks` and `make docker` both fail from a linked worktree. Filed as #33. ### Limits of verification, stated plainly - **CI job logs were not readable** — the Actions API returns 403 for this account. The reviewer has the commit status only (`success`). They compensated by reproducing both CI steps locally: root `make check` and the full `docker build -f Dockerfile.backend .`. I am satisfied by that, but the CI log itself is unverified. - **The Darwin branch is unexercised end to end.** No macOS host available. The `shasum -a 256` code path was tested via a make variable override on Linux and the output format confirmed compatible, but the `ifeq ($(UNAME_S),Darwin)` selection itself has never run. If you merge and later hit this on a Mac, that is the place to look. Over to you for the merge.
clawbot commented 2026-08-09 07:40:20 +02:00
Author
Collaborator
Copy Link

Re-verified against the cached-green risk — merge-ready stands

A fleet-wide warning came in after I labelled this: script/cibuild is a bare docker build . with no cache control, so on an unchanged tree Docker can serve RUN make check from cache and the build exits 0 having run nothing. Observed elsewhere in the org as a SUCCESS in 0.262s with every layer cached.

Given that this PR is specifically about a lint gate that had already been silently doing nothing, accepting a possibly-cached green here would have been the same mistake twice. So I re-verified directly rather than trusting the recorded evidence.

Forced-uncached build at head 4d70317

Fresh clone, --no-cache, full output:

=== forced UNCACHED build of Dockerfile.backend ===
#17 7.277 ?   sneak.berlin/go/netwatch/cmd/netwatch-server  [no test files]
#17 7.576 ok  sneak.berlin/go/netwatch/internal/handlers    0.003s
#17 7.576 ok  sneak.berlin/go/netwatch/internal/reportbuf   0.003s
#17 12.12 0 issues.
#17 DONE 12.4s
EXIT=0
ELAPSED=74s

74 seconds, real go test package timings, and golangci-lint running to 0 issues. under the pinned v2.12.2. The checks demonstrably executed. This green is earned. No retraction.

Also worth noting: the drift guard added by this PR ran inside that uncached build too, so the offline hash check is exercised on the real CI path, not just locally.

Corroborating evidence already in hand

Two things from the reviews independently rule out a cached green:

  1. Both reviewers ran docker run --network none against the built builder image. A docker run executes by definition — it cannot be served from a layer cache.
  2. The second reviewer's first cold build failed at backend/Makefile's timeout 30 go test ./... with an empty container build cache. A cached layer would not have run the test at all, let alone timed out. That failure is itself proof of genuine execution.

The hole is real for this repo, and is now filed

I reproduced it here rather than assuming netwatch was exempt. After one warm build, a repeat docker build . on an unchanged tree:

9 CACHED layers
ELAPSED_MS=514
#13 [build 7/7] RUN make check      <- CACHED, no vite output, no prettier output

514ms, exit 0, nothing ran. Filed as #37 and attached to 1.0.0, sequenced behind #16 since both rewrite script/cibuild.

That means the CI success status on 4d70317 is not by itself trustworthy evidence — but the forced-uncached build above is, and it is the basis on which I am keeping this labelled merge-ready.

What this changes going forward

I am treating a green CI status as insufficient evidence for any future PR in this repo until #37 lands. Reviewers will be instructed to demonstrate an uncached execution rather than cite the CI badge. Worth knowing that this repo has now had three independent ways to report an unearned green: an inert lint config (#14, fixed by this PR), a root make check that never touched the backend (#16), and a cacheable CI gate (#37).

## Re-verified against the cached-green risk — `merge-ready` stands A fleet-wide warning came in after I labelled this: `script/cibuild` is a bare `docker build .` with no cache control, so on an unchanged tree Docker can serve `RUN make check` from cache and the build exits 0 having run nothing. Observed elsewhere in the org as a SUCCESS in 0.262s with every layer cached. Given that this PR is specifically about a lint gate that had *already* been silently doing nothing, accepting a possibly-cached green here would have been the same mistake twice. So I re-verified directly rather than trusting the recorded evidence. ### Forced-uncached build at head `4d70317` Fresh clone, `--no-cache`, full output: ``` === forced UNCACHED build of Dockerfile.backend === #17 7.277 ? sneak.berlin/go/netwatch/cmd/netwatch-server [no test files] #17 7.576 ok sneak.berlin/go/netwatch/internal/handlers 0.003s #17 7.576 ok sneak.berlin/go/netwatch/internal/reportbuf 0.003s #17 12.12 0 issues. #17 DONE 12.4s EXIT=0 ELAPSED=74s ``` 74 seconds, real `go test` package timings, and `golangci-lint` running to `0 issues.` under the pinned v2.12.2. The checks demonstrably executed. **This green is earned. No retraction.** Also worth noting: the drift guard added by this PR ran inside that uncached build too, so the offline hash check is exercised on the real CI path, not just locally. ### Corroborating evidence already in hand Two things from the reviews independently rule out a cached green: 1. Both reviewers ran `docker run --network none` against the built builder image. A `docker run` executes by definition — it cannot be served from a layer cache. 2. The second reviewer's **first cold build failed** at `backend/Makefile`'s `timeout 30 go test ./...` with an empty container build cache. A cached layer would not have run the test at all, let alone timed out. That failure is itself proof of genuine execution. ### The hole is real for this repo, and is now filed I reproduced it here rather than assuming netwatch was exempt. After one warm build, a repeat `docker build .` on an unchanged tree: ``` 9 CACHED layers ELAPSED_MS=514 #13 [build 7/7] RUN make check <- CACHED, no vite output, no prettier output ``` 514ms, exit 0, nothing ran. Filed as **#37** and attached to `1.0.0`, sequenced behind #16 since both rewrite `script/cibuild`. That means the CI `success` status on `4d70317` is **not** by itself trustworthy evidence — but the forced-uncached build above is, and it is the basis on which I am keeping this labelled `merge-ready`. ### What this changes going forward I am treating a green CI status as insufficient evidence for any future PR in this repo until #37 lands. Reviewers will be instructed to demonstrate an uncached execution rather than cite the CI badge. Worth knowing that this repo has now had **three** independent ways to report an unearned green: an inert lint config (#14, fixed by this PR), a root `make check` that never touched the backend (#16), and a cacheable CI gate (#37).
All checks were successful
check / check (push) Successful in 1m13s
Required
Details
This pull request can be merged automatically.
You are not authorized to merge this pull request.
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin feat/golangci-standard-config:feat/golangci-standard-config
git checkout feat/golangci-standard-config
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
Clear labels
merge-ready
needs-checks
needs-rebase
needs-review
needs-rework
No Label merge-ready
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
clawbot sneak
No Assignees sneak
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/netwatch#31
Delete Branch "feat/golangci-standard-config"

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?