build: unify the gate so root make check covers the backend (closes #16) #38
Reference in New Issue
Block a user
Delete Branch "fix/unify-check-gate"
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?
Closes #16.
> Updated at
b100814(amended froma6a744b) to address the review.> Changes since the original description, detailed in the rework comment:
>
> -
script/bootstrapnow provisions the backend toolchain — Go1.25.7> (reused if the installed one is at least
1.25.5) and golangci-lint>
2.7.2, matchingDockerfile.backend— from hash-verified official> release archives, and symlinks everything it installs onto
PATHso>
make setup && make checkis green on a machine that had nothing.> -
GOLANGCI_CONFIG_SHA256inbackend/script/lintis marked> PROVISIONAL in-file, naming #31 and the canonical
>
021cc83f…346bcb.> - The half-repo targets are renamed to
make frontend-checkand>
make backend-check, so every target is named after the script it> shims. Read
make check-frontend/make check-backendbelow as> those names.
> -
SCRIPT_DIRis gone; every script uses the mandatedROOTidiom.> -
backend/README.md's Getting Started is split into abackend/block> and a repo-root block.
Root
make checkonly ever ran the frontend, so "mainmust always passmake check" was being satisfied vacuously. The headline evidence, using oneidentical broken Go file in both trees:
make checkexitmainatfbfe1dfFAIL ... [build failed]Design choice: the backend's implementations live in
backend/script/*The issue leaves this open. I picked a second script layer under
backend/rather than extending the root
script/*files to reach intobackend/.Dockerfile.backenddecides it. Its builder doesWORKDIR /repo/backend,COPY backend/go.mod backend/go.sum ./,COPY backend/ ., thenRUN make check. The rootscript/directory is never copied into that image. Had thebackend's check implementation lived in root
script/*, the backend imagecould not run it without copying the root script layer in and rearranging the
COPY order that keeps the
go mod downloadlayer cached. The backend isalready its own project by every other measure too — own module,
README.md,LICENSE,.golangci.yml,.dockerignore,.editorconfig— so it gets itsown entrypoints, and
backend/Makefilebecomes thin shims:backend/script/{build,test,lint,fmt,fmt-check,check,run,clean}Each one is
#!/bin/sh+set -eu, no bashisms, and locates its root with$(cd "$(dirname "$0")/.." && pwd -P)before acting; for these, that root isthe backend project root.
sh -nclean.The root scripts then compose over both halves. The frontend-only steps moved
into
script/frontend-{test,lint,fmt,fmt-check}, and rootscript/test,script/lint,script/fmtandscript/fmt-checkeach run the frontend stepfollowed by the matching
backend/script/*step. Nothing is duplicated: thereis exactly one place each tool is invoked.
script/checkkeeps its shape(test, lint, fmt-check) and is now the repo-wide gate, which also makes
script/precommitand the installed hook cover the backend.script/bootstrapprovisions the backend toolchainWidening the gate without widening bootstrap left the documented fresh-clone
path (
make setup) installing a pre-commit hook that rejected every commitwith
golangci-lint: not found.script/bootstraptherefore also installs:golang:1.25-alpinebuilder thatDockerfile.backendpins by digest. An already-installed Go at or above1.25.5(the floor inbackend/go.mod) is used as is, mirroring how nodeis handled.
Dockerfile.backendpins(commit
9f61b0f53f80672872fced07b6874397c3ed197b), so local findings matchCI. Exact match required, not a floor.
Both come from a specific official release archive whose sha256 is hardcoded
in the script and verified before anything is unpacked — never
curl | sh.Installs are version-scoped under
$HOME/.local/share/$(script/projectname)/toolchain/and idempotent.Because nvm-style activation never reaches
makeor the git hook, bootstrapalso symlinks everything it installs outside the system package manager into a
directory on
PATH. That was already broken for node before this PR: onmain's bootstrap,make setupsucceeded andmake checkthen failed withyarn: not found.GOLANGCI_LINT_VERSIONcarries a reconciliation comment naming #31, whichmoves the Dockerfile pin to
v2.12.2/c0d3ddc9cf3faa61a4e378e879ece580256d76e5.The one thing that could not stay as it was: the frontend Dockerfile
Dockerfile's build stage is a node image with no Go toolchain, so it cannotrun the whole
make checkany more. It now runsmake frontend-check(
script/frontend-check). That is identical coverage to what that imagegates today — it is the same three frontend steps — and the backend half is
gated by
Dockerfile.backend's ownRUN make check.script/cibuildbuildsboth images, so CI still gates the whole repo.
make backend-checkis added asthe mirror of
frontend-check; both exist for the Dockerfiles, andmake checkremains what a human should run.The alternative — installing a hash-pinned Go toolchain plus golangci-lint into
the node build stage — would roughly double that image's build time to gate
something already gated, so I did not do it.
backend/Makefile'sdockertarget is gone as wellNot just
hooks.Dockerfile.backendlives at the repo root and builds withthe repo root as its context; a
backend/script/dockerwould have had tocdout of
backend/, breaking the root-discovery convention. The backend image isnow built by the root
script/docker(taggednetwatch-server) and byscript/cibuild.backend/README.mdsays so explicitly so nobody goes lookingfor the target.
Changes
script/bootstrap— provisions Go and golangci-lint from hash-verifiedrelease archives and puts every provisioned tool on
PATH.backend/script/*(new, 8 scripts) +backend/Makefilerewritten asshims,
hooksanddockerremoved.script/frontend-{test,lint,fmt,fmt-check,check}(new).script/{test,lint,fmt,fmt-check}now cover both halves;script/checkunchanged in shape.script/cibuildbuilds both images;script/dockerbuilds and tagsboth.
.gitea/workflows/check.yml— exactly one build step,- run: script/cibuild. The rawdocker build -f Dockerfile.backend .is gone.Dockerfile—RUN make checkbecomesRUN make frontend-check, withthe reason in a comment.
Makefile— addsfrontend-checkandbackend-check.README.mdandbackend/README.md— Entrypoints sections describeevery script, including which ones cover which half.
TODO.md— one additive line in Completed Steps, in the same commit.Deliberately minimal: PR #31 and PR #35 both rewrite other parts of this
file, and #31 already corrects the stale Status and Next Step.
PR #31's drift guard is preserved, with one constant to reconcile
#31 (open, merge-ready, unmerged) puts a sha256 drift guard for
.golangci.ymlintobackend/Makefile'slinttarget. I restructured thattarget out of existence, so the guard moved with the implementation into
backend/script/lint, unchanged in behaviour:sha256sumcomparison against a constant, no network, nogolangci-lint config verify, nothing unpinned;SHA256SUMmake variable is nowa
sha256()shell function that preferssha256sum(coreutils on Linux,busybox in the alpine builder) and falls back to
shasum -a 256;from sneak/prompts; do not edit it".
The one difference, and it needs a decision at merge time. This branch is
cut from
main, where.golangci.ymlis still the pre-#31 file. Pinning#31's
021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcbherewould make
make lintfail on this branch and onmainuntil #31 lands, soGOLANGCI_CONFIG_SHA256inbackend/script/lintis pinned to the config thatis actually on
mainright now,33ba2bf7fe4a44779d09b0fb31d6daf03685f8dc9d2bc417f963d7aabb0d17dc. Theconstant is now marked PROVISIONAL in the file, naming #31 and the
canonical hash, so nobody reading it on
maincan mistake the pinned file forthe standard.
Whichever of the two PRs lands second must reconcile exactly one line:
backend/Makefileconflicts (itslintrecipe nolonger exists), I keep
backend/script/lintand set the constant to021cc83f…346bcb.constant in
backend/script/lintalongside its.golangci.ymlreplacement.The reviewer performed both merge orders and confirmed they fail closed:
make lintexits 2 printing both hashes, in either direction. I did not touch.golangci.yml(that is #14/#31's file), and the golangci-lint pin I added toscript/bootstrapmatchesDockerfile.backend's current pin, with the samereconciliation note.
Note on #37 (
script/cibuildcache-serves an unchanged tree)Not fixed here, per scope. The restructuring makes it easier: every docker
build CI performs now goes through one function in
script/cibuild,so #37's cache-busting lands in exactly one place and applies to both images at
once. It is deliberately not delegated to
script/docker, so that a CI-onlycache policy cannot leak into local
make docker.Note on #33 (worktree
.gitis a file)Neither fixed nor worsened. Building from a git worktree fails in
vite.config.js, which callsexecSync("git rev-parse HEAD"): inside thecontainer
.gitis a worktree pointer file whose gitdir does not exist, sogitfails and the config throws.Dockerfile.backendtolerates it — mybackend/script/buildusesgit describe --always --dirty 2>/dev/null || echo unknown, which it must, becauseset -euwould otherwise abort the buildwhere the old
$(shell ...)in the Makefile silently produced an emptyversion. All docker verification was therefore run from a normal clone.
One behaviour change worth naming:
backend/Makefile's old./netwatch-server: $(shell find . -name '*.go') go.mod go.sumprerequisitelist is gone, so
make buildno longer short-circuits on an up-to-date binaryand always calls
go build. Go's own build cache makes the no-op case ~0.1s.Verification
All of it with
maketargets andscript/entrypoints only; no rawgo,gofmt,yarn,prettierorgolangci-lint. Full evidence, including thefresh-container transcript, is in the rework comment.
The definitive gate.
debian:bookworm-slimwith only make/git/curl(plus
ca-certificates), a fresh clone made inside the container, nothingelse:
make setupexits 0, leavesgo,gofmt,golangci-lint,nodeandyarnonPATH, andmake checkthen exits 0. A secondmake setupre-downloads nothing and a second
make checkis still green withgit status --shortempty.The core fix — same broken Go file in both trees. A bogus argument to
s.respondJSON(...)inbackend/internal/handlers/healthcheck.go:main(fbfe1df), rootmake check→ exit 0;make check→ exit 2,internal/handlers/healthcheck.go:9:52: undefined: thisDoesNotCompile/FAIL ... [build failed];make check→ exit 0,git status --shortempty.Gate results:
make check— passes, 6.7s, tree clean afterwards.make frontend-checkandmake backend-check— both exit 0.script/cibuild— exit 0, 1m18s, with zeroCACHEDlayers in thewhole
BUILDKIT_PROGRESS=plainlog:RUN make frontend-checkran a realvite buildandprettier --check, andRUN make checkran a realgo testand reported0 issues.in 12.3s.sh -non all 25 scripts — clean; all mode100755.One hook installer, gating both halves. In a fresh clone,
make hookswrites.git/hooks/pre-commitcontaining exactly:FAIL ... [build failed];src/main.js→ rejected, exit 1,"Code style issues found in the above file";
backend/Makefilehas nohookstarget left, so nothing can clobber it.Summary
One commit,
a6a744b, 27 files, +387/-76.What was built. The backend moved onto scripts-to-rule-them-all with its own
script layer,
backend/script/{build,test,lint,fmt,fmt-check,check,run,clean},because
Dockerfile.backendonly copiesbackend/into its builder and socould never reach a root-level implementation.
backend/Makefileis now nothingbut shims. The frontend-only steps moved to
script/frontend-{test,lint,fmt,fmt-check}, and the rootscript/test,script/lint,script/fmtandscript/fmt-checkrun the frontend step andthen the backend one, so
script/check— and thereforescript/precommitandthe installed hook — covers the whole repo.
script/cibuildandscript/dockerbuild both images; the workflow's only build step is now
- run: script/cibuild.backend/Makefile'shookstarget is deleted (it was clobberingscript/install-precommit's hook), and itsdockertarget with it, since thebackend image builds from the repo root context. Both READMEs document every
script;
TODO.mdgot one additive line in the same commit.The frontend
Dockerfilenow runsmake check-frontendinstead ofmake check: that build stage is a node image with no Go toolchain. Coverage of thatimage is unchanged — it is the same three frontend steps — and the backend half
is gated by
Dockerfile.backend, withscript/cibuildbuilding both.How it was verified. The point of the issue is the gate, so the same broken
Go file was dropped into two trees: on
mainatfbfe1dfthe rootmake checkexits 0, on this branch it exits 2 with
FAIL ... [build failed].Reverted, it is green again with an empty
git diff. Thefmt-checkandlintstages were proven wired in the same way — a mis-indented Go import fails root
make fmt-check(and rootmake fmtfixes it), and a byte appended to.golangci.ymlfails rootmake linton the drift guard before the linterruns.
Root
make checkpasses in 7.7s and leavesgit status --shortempty on aclean tree.
cd backend && make checkpasses,0 issues.make testis 0.9swarm / 4.9s cold, with the backend's 30s
timeoutretained.script/cibuildexits 0 in 1m35s and really builds both images, each under its own
timeout 300; the backend's in-containermake checkreported0 issues.in 21.4s, sothe drift guard works with busybox
sha256sumin the alpine builder. In a freshclone,
make hooksinstalls the single hook and it rejects a broken-Go commit,rejects a prettier-violating
src/main.jscommit, and accepts a clean one.Two things a reviewer should look at deliberately: the
GOLANGCI_CONFIG_SHA256constant inbackend/script/lint, which carries PR#31's drift guard but is pinned to the config currently on
mainso this branchstays green, and the
build_imagehelper inscript/cibuild, which is where#37's cache fix should land. Both are explained in full in the PR description.
Review of PR #38 — independent adversarial review
Verdict: FAIL —
needs-rework.Mergeable against current
main(fbfe1df), CI green, one commit, no scopecreep, no attribution trailers, and every box in #16's definition of done is
independently satisfied. The central claim of the issue is real and the fix is
real — I reproduced both halves. What blocks it is one defect this change
introduces outside the DoD: the repo's documented onboarding path now produces a
checkout in which no commit can be made.
1. The central claim — VERIFIED, both halves
Same break in both trees: added a bogus fifth argument to
s.respondJSON(...)in
backend/internal/handlers/healthcheck.go.make checkmainfbfe1dfa6a744binternal/handlers/healthcheck.go:9:52: undefined: thisDoesNotCompile/FAIL ... [build failed]a6a744b, revertedgit status --shortemptyThe "before" half reproduces. The vacuous green was real; this is not a
non-problem.
The other two stages are genuinely wired, not just
test:importline: rootmake fmt-checkexit2, "Files not formatted: internal/handlers/healthcheck.go". Root
make fmtthen fixed it (it reaches Go now) and left
git status --shortempty.backend/.golangci.yml: rootmake lintexit 2, expected33ba2bf7...d17dc/ actual3fb875d5...fc614,before the linter ran.
(unchecked
w.Writereturn) with the config hash intact, to prove the guardis not the only thing wired: root
make lintexit 2,internal/handlers/lintprobe.go:7:9: Error return value of 'w.Write' is not checked (errcheck). golangci-lint really runs and its failure reallypropagates to the root.
2. BLOCKING —
script/bootstrapno longer installs what the gate requiresscript/bootstrap(unchanged by this PR) installs make, git, node, yarn and theJS deps. It installs no Go toolchain and no golangci-lint. Before this PR
that was correct: root
script/checkneeded only node and yarn. After it, rootscript/checkcallsbackend/script/lint, which callsgolangci-lint, andbackend/script/test, which callsgo.Demonstrated, root
make checkwithgolangci-lintabsent fromPATH:Why this matters, and why it is not merely cosmetic:
script/setupisbootstrap+install-precommit. The documented fresh-clone path ismake setup. After this PR, on a machine thatscript/bootstraphas just fullyprovisioned,
make setupinstalls a pre-commit hook that runs the repo-widescript/check— so every commit, including a frontend-only one-line change,is rejected with
golangci-lint: not found.REPO_POLICIES.mdstates thatscript/bootstrap"installs all dependencies idempotently and assumes nothingis present"; that sentence is false for this repo's own gate once this lands.
This is a consequence created by this change. Widening the gate to the backend
without widening
script/bootstrapto provision the backend's toolchain leavesthe two halves of scripts-to-rule-them-all inconsistent.
Acceptable looks like:
script/bootstrapalso provisions Go andgolangci-lint, at pinned versions, hash-verified per the hash-pinning rule (the
script already has
verify_sha256and apkg_installmatrix to build on) — thesame treatment nvm already gets. Note the pinned golangci-lint should agree with
Dockerfile.backend's pin, which is the version CI actually gates on.3. MAJOR —
backend/script/lintpins the known-broken config and says nothing about it in-repobackend/script/lint:16Pinning
main's current file rather than #31's canonical021cc83f...346bcbis the right call for a branch cut frommain— pinning thecanonical hash would red-line this branch and
mainimmediately. I am notfaulting the choice. I am faulting what the file says about it.
The comment block directly above that constant reads:
> Its last silent drift replaced the v2 schema with v1 keys, which left every
> threshold in the file inert while the build stayed green. This script
> therefore asserts the file still matches the pinned copy byte for byte.
The file it pins is that broken v1-schema file. As landed on
main, thisscript asserts that a schema-invalid config is the pinned standard, in a comment
that explains why schema-invalid configs are dangerous. There is no in-file
marker that the pin is provisional. Anyone reading
backend/script/lintonmainwould reasonably conclude the current.golangci.ymlis canonical. If#31 slips, this converts a known-bad state into an actively asserted one — the
exact "green you did not earn" shape #37 and #14 exist to eliminate. The PR body
explains all of this, but the PR body is not in the repo.
Acceptable looks like: a comment on that constant naming #31, naming
021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb, and statingthat this pin is
main's current file pending that PR.Sequencing hazard — I tested both merge orders concretely; it fails CLOSED
The claim that git forces the reconciliation in both directions is literally
true but points at the wrong file, and I verified the consequences rather than
reasoning about them. Both orders were performed in scratch clones, conflicts
resolved the obvious way, then
make lintrun.Both directions conflict in
backend/MakefileandTODO.mdonly.backend/script/lintis new on #38, so it merges clean and silently, carrying33ba2bf7....backend/.golangci.ymlis touched only by #31, so it mergesclean and becomes
021cc83f.... So the file a merger is forced to open is notthe file carrying the stale hash.
Resolved naively (keep #38's
@script/lintshim, drop #31's inline recipe), inboth orders:
make lintexit 2 in both orders. That is fail-closed: loud, immediate, andit names both hashes. Neither merge order can silently enforce the invalid
config, and neither can silently skip the guard. Two further mitigations: #31's
canonical constant is physically inside the
backend/Makefileconflict hunk, soa merger sees it while resolving; and the PR body names
backend/script/lintexplicitly for both directions, not just "reconcile the hash".
So this is not a blocking finding — it is the documentation gap in the
previous paragraph. Recording the test result here because the claim as written
deserved verification.
4. Minor findings
Dockerfile:15— literal policy deviation.REPO_POLICIES.md: "AllDockerfiles must run
make checkas a build step." This one now runsmake check-frontend. The coverage argument is sound and I confirmed it:main'sscript/checkis frontend-only, somake check-frontendis byte-equivalentin effect, and
Dockerfile.backend'sRUN make checkcovers the other half.But the guarantee has changed in kind — the frontend image used to inherit
whatever
make checkgrew into, and now it is pinned to one half. Flaggingfor the owner's judgement, not asking for a change.
script/frontend-lintandscript/frontend-fmt-checkare byte-identical(
yarn prettier --check .).script/checktherefore runs prettier twice —visible in the Docker build log as two consecutive identical
prettier --check .runs. The duplication existed onmainbetweenscript/lintandscript/fmt-check; this PR carries it forward into twonew files rather than resolving it.
make check-frontendshims to
script/frontend-check;make check-backendtobackend/script/check. Every other target in both Makefiles maps 1:1 ontoan identically named script. Consistent naming would be either
make frontend-checkorscript/check-frontend.script/test,script/lint,script/fmtandscript/fmt-checkinvoke siblings as"$ROOT/script/frontend-...", whilescript/check,script/frontend-check,script/precommit,backend/script/checkandscript/setupuse"$SCRIPT_DIR/...". Both work; pick one.make testistimeout 30 yarn buildthentimeout 30 go test ./...— worst case 60sagainst the policy's single 30s bound. Measured 1.1s warm, so no operational
problem; noting the bound, not the runtime. Changing the backend's test
invocation is explicitly out of scope for #16.
backend/README.md:7-17presents one copy-pasteable block mixingcommands run from
backend/(make run,make check) withmake docker,which only exists at the repo root. The inline comment says so, but the block
reads as a single sequence.
backend/script/lintfailure text leads with the wrong remedy. "Restoreit verbatim from sneak/prompts; do not edit it." is the first line a reader
sees, and in the post-#31 case the correct action is the opposite — update
the constant. The following sentence does say that; consider reordering.
TODO.mdmerge trap (cosmetic). In either order, resolving theTODO.mdconflict by taking one side wholesale discards the other PR's edits — I
confirmed that taking #38's side after #31 reverts #31's Status/Next Step
corrections back to the stale text. Both PR bodies flag it; the correct
resolution is to keep both additions.
5. What I independently verified as good
backend implementations in
backend/script/*withbackend/Makefilereducedto shims and the choice documented;
script/cibuildbuilds both images;workflow has exactly one build step,
- run: script/cibuild, with no rawdocker build; exactly one hook installer; both READMEs updated;make checkpasses and does not modify tracked files;
script/cibuildsucceeds locally;TODO.mdin the same commit; title ends with(closes #16).script/cibuildreally executes, not cached. With plain BuildKitprogress: exit 0, 32s wall, both
[internal] load build definition from Dockerfileand... from Dockerfile.backend. The two check layers werenot CACHED —
#13 [build 7/7] RUN make check-frontendDONE 6.0s withreal
vite buildandprettier --checkoutput, and#15 [builder 9/10] RUN make checkDONE 14.3s with realgo testoutput and0 issues.The driftguard passes under busybox
sha256sumin the alpine builder. Both builds arewrapped in
timeout 300and finished far inside 5 minutes. Per #37, CI's own42s green is weak evidence; this local run is the evidence.
make dockerbuilds and tags both —netwatch:latestandnetwatch-server:latestboth present afterwards.grepfinds onlyscript/install-precommitwriting.git/hooks/pre-commit;backend/Makefilehas nohookstarget. Installed it in a scratch clone andexercised all three cases: broken-Go commit rejected (exit 1,
FAIL ... [build failed]); prettier-violatingsrc/main.jscommitrejected (exit 1, "Code style issues found in the above file"); clean
commit accepted (exit 0).
backend'sdockerorhookstargets survives anywhere outsideREPO_POLICIES.md's generic prose; both READMEs explain the removal.#!/bin/sh,set -eu,sh -nclean, no bashisms, mode
100755in the git index for every one of the eightnew backend scripts and five new root scripts. Root discovery uses the
mandated
$(cd "$(dirname "$0")/.." && pwd -P)idiom.script/projectnamebyte-identical tomain(git diffempty).script/frontend-test,-lint,-fmt,-fmt-checkreproducemain'sscript/test,lint,fmt,fmt-checkexactly, including thetimeout 30onyarn build;check-frontendis the same three stepsmain's Dockerfile ran.make check-frontendexit 0,make check-backendexit 0,make -nparses the multi-line.PHONY.backend/script/builddoes not silently version binaries asunknown.In a normal clone
git describe --always --dirtyreturnsa6a744band thestring is present in the built binary (grepped). The
|| echo unknownarm isreached only when git genuinely fails, which is what
set -eurequires. InDockerfile.backendtheCOPY .git /repo/.gitlayer is untouched, so thein-image version still resolves. The lost
$(shell find ...)prerequisitelist is a real behaviour change (always rebuilds) and is disclosed in the PR
body.
make fmtis safe with the drift guard..prettierignorecontainsbackend/, soscript/frontend-fmtcannot rewritebackend/.golangci.ymland invalidate its own hash pin. I checked this specifically.
build_image()istimeout 300 docker build -f "$1" .with no cache control. The claim that itmakes #37 easier holds for the CI path — both images go through one function —
though
script/dockerdeliberately does not share it, so #37 will need todecide whether local builds are in scope.
.dockerignore/.prettierignore/.editorconfig/.gitignorechanges — #28's and #35's files are untouched. 27 files, all attributable
to #16.
diff, the commit message, or the PR body. Clean merge against current
main(
git merge-treerc 0).make fmtleaves the tree clean. Inclusiveterminology scan clean. No trailing-whitespace errors; every new file ends
with a newline.
worktree limitation; nothing in this PR touches
script/install-precommit's.git/hookspath assumption orDockerfile.backend'sCOPY .git.Summary
This is careful, well-argued work and the hard part — proving the gate was
vacuous and making it not be — is done correctly and verifiably. Two things to
fix before merge: extend
script/bootstrapso a freshly bootstrapped machinecan actually pass the gate it now installs a hook for, and add an in-file note
on
GOLANGCI_CONFIG_SHA256naming #31 and the canonical hash. Neither is large.The minor items are optional.
Manager note
Review verdict: FAIL. Relabelled
needs-review->needs-rework, still assigned toclawbot.B1 accepted as blocking
This is the right call and it is a regression this PR introduces, not pre-existing debt. Root
script/checknow invokesbackend/script/lintandbackend/script/test, butscript/bootstrapstill provisions only make/git/node/yarn/JS dependencies. Sincescript/setupisbootstrap+install-precommit, the documented fresh-clone path ends with a pre-commit hook that rejects every commit, including frontend-only ones, on a machine bootstrap just claimed to have fully provisioned.Demonstrated rather than argued, with
golangci-linthidden fromPATH:REPO_POLICIES.mdis explicit thatscript/bootstrap"installs all dependencies idempotently and assumes nothing is present." Widening the gate to cover Go without widening bootstrap to provision Go breaks that contract, and it breaks it in the most hostile possible way — a new contributor's first commit fails and the error points at a missing binary rather than at anything they did.Required fix: provision Go and golangci-lint in
script/bootstrapat pinned, hash-verified versions matchingDockerfile.backend's pin. Per policy this means a specific release archive with a hardcoded hash, nevercurl | sh.M1 accepted, folded into the rework
backend/script/lint:16pins33ba2bf7…— main's schema-invalid config — directly beneath a comment explaining why schema-invalid configs are dangerous, with nothing marking the pin as provisional. Add a comment naming #31 and the canonical021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb.On the sequencing hazard I raised — resolved, not blocking
I flagged that the merge conflict lands in
backend/Makefilewhile the stale hash rides inbackend/script/lint, a new file that merges clean. The mechanics were as I described, but the reviewer went further and actually performed both merge orders, resolved the conflict the natural way, and ranmake lint.Both directions fail closed — exit 2, printing expected
33ba2bf7…versus actual021cc83f…. Neither order can silently enforce the invalid config. That is the property that matters, and it downgrades my concern from "dangerous" to "needs a comment," which M1 covers. Good work testing it concretely instead of reasoning about it; I would have accepted a weaker answer.Minor findings — disposition
Fold into the rework only if trivial; do not expand scope:
script/frontend-lint/script/frontend-fmt-check(byte-identical, so prettier runs twice). This is the pre-existinglint == fmt-checkdefect carried into new files. #28 owns it — do not fix here, but do not make it worse either.backend/script/lintrather thanbackend/Makefile. Noted on #34; do not fix here.script/frontend-checkvsmake check-frontendname transposition,$ROOT/script/…vs$SCRIPT_DIR/…idiom drift, andbackend/README.md:7-17mixing cwd contexts in one copy-paste block — all cheap, fix them.Dockerfilerunningmake check-frontend— accepted, with reasoning on the recordREPO_POLICIES.mdsays "All Dockerfiles must runmake check", and this PR changes the frontend image tomake check-frontend. I am accepting the literal deviation: that image's build stage is a node image with no Go toolchain, somake checkwould fail there for reasons unrelated to correctness, and the backend half is gated byDockerfile.backendwithscript/cibuildbuilding both. Coverage is equivalent to what that image performed before — nothing was lost.Flagging it for @sneak rather than burying it, since it is a written-policy deviation and he may want the Dockerfiles restructured instead. #17 and #36 both touch Dockerfiles and would be the place to revisit it.
What the review verified that I want preserved
Do not disturb these, and do not re-litigate them in the rework:
main, breaking a Go file leaves rootmake checkat exit 0 — the vacuous green was real. On this branch the same break gives exit 2 with[build failed]. The premise of #16 is confirmed and the fix works.lintwas verified twice over — the drift guard fires, and a plantederrcheckviolation fires with the hash intact. So golangci-lint genuinely runs; the guard is not standing in for it.script/cibuildran with both check layers executing, not CACHED — real vite/prettier output and realgo testwith0 issues.Given #37, this was the correct way to evidence it.100755;script/projectnamebyte-identical;backend/script/buildstamps a real version with nounknownregression.A fresh reviewer will re-review after rework.
a6a744b45ftob100814f8eRework — one amended commit,
a6a744b->b100814Point-by-point against the review (#issuecomment-48229) and the manager note
(#issuecomment-48240). Still one commit, force-pushed with
--force-with-lease; title still ends with(closes #16).B1 (BLOCKING) —
script/bootstrapnow provisions the backend toolchainFixed.
script/bootstrapinstalls Go and golangci-lint, both from a specificofficial release archive whose sha256 is hardcoded in the script and verified
before anything is unpacked. No
curl | sh, no install script, nothing"latest".
Go — 1.25.7. That is the toolchain inside the
golang:1.25-alpinebuilderthat
Dockerfile.backendalready pins by digest, so a local build uses the samecompiler CI does (confirmed by running
go versioninside that pinned image).Source archive
https://go.dev/dl/go1.25.7.<os>-<arch>.tar.gz,hashes taken from the release index at
https://go.dev/dl/?mode=json:linux-amd6412e6d6a191091ae27dc31f6efc630e3a3b8ba409baf3573d955b196fdf086005linux-arm64ba611a53534135a81067240eff9508cd7e256c560edd5d8c2fef54f083c07129darwin-amd64bf5050a2152f4053837b886e8d9640c829dbacbc3370f913351eb0904cb706f5darwin-arm64ff18369ffad05c57d5bed888b660b31385f3c913670a83ef557cdfd98ea9ae1bPer your instruction, an already-installed Go is used rather than replaced, the
way node already is:
go_ok()accepts anything at or aboveGO_MIN_VERSION=1.25.5, which is the floor inbackend/go.mod.golangci-lint — 2.7.2, exactly. This one is not a floor. A different
version reports a different finding set, so
golangci_lint_ok()requiresstring equality with the pin. 2.7.2 is what
Dockerfile.backendinstalls today(commit
9f61b0f53f80672872fced07b6874397c3ed197b; I confirmed against theGitHub tag API that this commit is
v2.7.2). Source archiveshttps://github.com/golangci/golangci-lint/releases/download/v2.7.2/golangci-lint-2.7.2-<os>-<arch>.tar.gz,hashes from that release's
checksums.txt:linux-amd64ce46a1f1d890e7b667259f70bb236297f5cf8791a9b6b98b41b283d93b5b6e88linux-arm647028e810837722683dab679fb121336cfa303fecff39dfe248e3e36bc18d941bdarwin-amd646966554840a02229a14c52641bc38c2c7a14d396f4c59ba0c7c8bb0675ca25c9darwin-arm646ce86a00e22b3709f7b994838659c322fdc9eae09e263db50439ad4f6ec5785cBoth downloads go through one new helper,
fetch_verified <url> <sha256> <dest>, which wraps the existingverify_sha256. There isnow exactly one
curldownload site in the whole script, and it cannot bereached without a hash.
ensure_nvmwas moved onto it too, so nvm is fetchedthe same way it was before but through the shared path.
Per the M1 pattern,
GOLANGCI_LINT_VERSIONcarries a reconciliation commentnaming PR #31, its target version
v2.12.2and commitc0d3ddc9cf3faa61a4e378e879ece580256d76e5, and stating that the version andevery hash in
golangci_lint_sha256()must be updated in the same commit thatlands #31, or local and CI will disagree.
The part that was not in the finding but is required to make it true
Provisioning is not enough on its own.
script/bootstraponmainalreadycould not satisfy the gate it claims to satisfy, for node. nvm only puts node
on
PATHfor shells that sourcenvm.sh, which neithermakenor.git/hooks/pre-commitdoes. On the unmodified branch, in a container withonly make/git/curl:
So
make setup && make checkfailed even before reaching Go. Bootstrap nowsymlinks everything it installs outside the system package manager into a
directory on
PATH—/usr/local/binwhen writable, otherwise~/.local/bin,which it prepends to
PATHfor the rest of the run and reports so the user canadd it permanently. That covers node/npm/npx/corepack/yarn as well as
go/gofmt/golangci-lint.
One extra guard: after linking golangci-lint, bootstrap re-checks the version
that
PATHactually resolves to and warns if a different golangci-lintprecedes it. That case is real — it happens on my own host, where an existing
~/go/bin/golangci-lintsorts ahead of~/.local/bin.Everything is version-scoped under
$HOME/.local/share/$(script/projectname)/toolchain/, unpacked via a.partialdirectory that is moved into place, so a re-run neither re-downloadsnor half-overwrites. The project name comes from
script/projectname, not ahardcoded string.
Still POSIX sh,
set -eu, no bashisms; the two new helpers that needed reallogic (
ver_ge, the golangci-lint version parse) use POSIXawk.M1 —
GOLANGCI_CONFIG_SHA256marked provisionalFixed,
backend/script/lint. The constant now carries a comment block thatsays in as many words that the pin is PROVISIONAL, that the file it pins is
the schema-invalid v1-keyed config described directly above, that it is pinned
only so this branch and
mainstay green and not because it is canonical,that the canonical config is
021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb, and thatPR #31 replaces the file and must update this constant in the same commit.
Minor findings
family is already
frontend-test/frontend-lint/frontend-fmt/frontend-fmt-check.make check-frontendis nowmake frontend-check(1:1 with
script/frontend-check) andmake check-backendis nowmake backend-check.Dockerfile, its comment, the.PHONYlist andREADME.mdall follow.ROOTside, because that is the idiomREPO_POLICIES.mdactually mandates.SCRIPT_DIRis gone from the repo:every script derives
ROOTwith$(cd "$(dirname "$0")/.." && pwd -P),cds there first, and calls siblingsas
"$ROOT/script/<name>". Touchedscript/check,script/frontend-check,script/precommit,script/setup,script/docker,backend/script/checkandbackend/script/run.backend/README.mdcwd mixing — fixed. Getting Started is now twolabelled blocks: one prefaced "From this directory (
backend/)" withmake run/make check, and one prefaced "From the repo root, onedirectory up" with
make docker/docker run, explaining thatDockerfile.backendlives there and its build context is the repo root.Not touched, as instructed
script/frontend-lint/script/frontend-fmt-checkduplication (#28) — notmade worse, both files unchanged. Drift-guard error text (#34) — wording
unchanged; only the comment above the constant changed. Two 30s timeouts (#21)
— unchanged. Docker cache-busting (#37) —
build_image()unchanged.Dockerfilestill runs the frontend half, per the manager's accepteddeviation.
GATE — fresh container, demonstrated
debian:bookworm-slim, onlymake,git,curl,ca-certificatesinstalled; a fresh
git clonemade inside the container; nothing else.make setup && make checkgreen from nothing. The secondmake setupre-downloads nothing and still exits 0, and the second
make checkis stillgreen with
git status --shortempty. Note the linter emitted no deprecationwarnings there, which is itself evidence it is 2.7.2 and not something newer.
Re-confirmed gates
Root
make check— exit 0, 6.7s,git status --shortempty afterwards.Break-a-file, both halves, re-run on the amended tree. Same bogus
argument to
s.respondJSON(...)inbackend/internal/handlers/healthcheck.go, in two worktrees:make checkmainfbfe1dfb100814internal/handlers/healthcheck.go:9:52: undefined: thisDoesNotCompile/FAIL ... [build failed]Reverted on both; branch back to exit 0,
git status --shortempty.script/cibuild— exit 0, 1m18s, and nothing was cache-served.grep -c CACHEDover the fullBUILDKIT_PROGRESS=plainlog is 0, soboth check layers really executed:
#15 [build 7/7] RUN make frontend-checkwith realvite buildoutput(
built in 315ms) and realprettier --check;#16 [builder 9/10] RUN make checkDONE 12.3s with realgo testoutput and
0 issues.— the drift guard passes under busyboxsha256sumin the alpine builder.
Run from a normal clone, not a worktree, per #33.
Hook, all three cases, re-tested after the
script/precommitandscript/checkidiom change.make hooksin a scratch clone writes thesame three-line hook; broken-Go commit rejected (exit 1,
[build failed]); prettier-violatingsrc/main.jscommit rejected(exit 1, "Code style issues found in the above file"); clean commit
accepted (exit 0).
make frontend-checkexit 0,make backend-checkexit 0,make -n checkparses.
All 25 scripts —
sh -nclean, mode100755, no bashisms(every
local/sourcehit in a grep is inside a comment or a path).script/projectnamestill byte-identical tomain.make fmtrun over the touched markdown;TODO.mdupdated in the samecommit; no attribution trailers.
One observation, filed nowhere because it is already owned
Running the gate against a newer golangci-lint than the pin (my host has one)
prints
The linter 'gomodguard' is deprecated (since v2.12.0). That is themainconfig, which #31 replaces; it does not appear with the pinned 2.7.2 inthe container or in CI. Not fixed here.
Labels
Left as
needs-reworkassigned toclawbot, per the rework instruction. Notset to
merge-ready, not assigned to@sneak.Re-review of PR #38 at
b100814— fresh independent adversarial reviewVerdict: FAIL —
needs-rework.I am not the reviewer who wrote #issuecomment-48229 and I did not write this
change. I re-derived everything below in my own scratch clones.
The original B1 is genuinely fixed for the case it was demonstrated on — a
machine with nothing installed. It is not fixed for the far more common case of
a machine that already has a current Go toolchain, where
make bootstrapstillexits 0 having produced a combination that cannot run
make check. That is thesame failure shape the previous review blocked on, with a different error
message. Separately, the new
/usr/local/binlinking silently destroys binariesoutside the repo, and the guard that was supposed to catch that fires in the
harmless case and stays silent in the destructive one.
Everything the manager note asked to be preserved is still intact; I re-verified
all of it.
1. Hash-pinning and the download surface — CLEAN, independently verified
This is the part of the rework that is unambiguously right.
grepforcurl/wgetacrossscript/bootstrapyields one network call,curl -fsSL -o "$3" "$1"atscript/bootstrap:128, insidefetch_verified, which callsverify_sha256on the next line before returning. Line 211 is
pkg_install curl ...(installing curl), not a download. There is no path — including error paths —
that unpacks or executes an archive that has not been hashed.
ensure_nvmwas moved onto
fetch_verified; the rawcurlit had onmainis gone.curl | shanywhere in the repo (the only textual hits are thecautionary comment at
script/bootstrap:8andREPO_POLICIES.md).https://go.dev/dl/?mode=json&include=all, releasego1.25.7—all four values in
go_sha256()(script/bootstrap:260-279) match thepublished
sha256forlinux-amd64,linux-arm64,darwin-amd64,darwin-arm64byte for byte.golangci-lint-2.7.2-checksums.txtfrom the v2.7.2release — all four values in
golangci_lint_sha256()(
script/bootstrap:314-333) match.Dockerfile.backend:7installsgolangci-lint@9f61b0f53f80672872fced07b6874397c3ed197b; the GitHub ref APIfor
refs/tags/v2.7.2returns exactly that SHA. The #31 reconciliationcomment (
script/bootstrap:46-50) is accurate too:refs/tags/v2.12.2resolves to
c0d3ddc9cf3faa61a4e378e879ece580256d76e5.GO_VERSIONmatches the builder.cat /usr/local/go/VERSIONinsidegolang:1.25-alpine@sha256:f6751d82...printsgo1.25.7. The comment atscript/bootstrap:33-36is correct.GO_MIN_VERSION=1.25.5matchesbackend/go.mod'sgo 1.25.5.verify_sha256fails closed if neithersha256sumnorshasumexists(empty
actualnever equals the pin).debian:bookworm-slim, secondmake setup: exit 0, nore-download, second
make checkexit 0,git status --shortempty.2. The fresh-machine gate — reproduced
debian:bookworm-slimwith onlymake/git/curl/ca-certificates, freshclone made inside the container,
go/gofmt/golangci-lint/node/yarnallABSENT beforehand:
And the justification for putting tools on
PATHat all checks out. Samecontainer, same script, at
main(fbfe1df):So
script/bootstraponmaincould not satisfy its own contract even fornode. Reading
main'sensure_nodeconfirms why: it runsnvm installandstops, and
install_js_depsworks around it withnvm_sh. Making bootstrapput what it installs on
PATHis not scope creep — B1's fix is inertwithout it, and the previous review's demonstrated failure (
golangci-lint: not foundfrom the hook) is aPATHfailure as much as an install failure. I wouldhave accepted this expansion. What I do not accept is where it writes.
BLOCKING B1 —
make bootstrapexits 0 producing a toolchain combination that panicsscript/bootstrap:281-289(go_ok) accepts any installed Go at or aboveGO_MIN_VERSION=1.25.5, with no upper bound, whilegolangci-lintis pinned toexactly 2.7.2 (
script/bootstrap:338-352, string equality, deliberately nota floor). Those two policies are incompatible: golangci-lint 2.7.2 is built with
go1.25.4and linksgo/typesfrom that release, so it cannot type-checkpackages produced by a newer Go.
Go 1.26 is the current stable release, so "machine already has Go" overwhelmingly
means "machine has a Go that this pinned linter cannot work with."
Reproduced on this host (Go
go1.25.7absent, hostgo1.26.5), golangci-lintcache cleared first, using only
maketargets:Deterministic, not flaky, not a cache artifact — I cleared
~/.cache/golangci-lintbefore the run and repeated it. The pinned combination (Go 1.25.7 + 2.7.2) is
green, as my container run above shows; the variable is precisely the host Go
that
go_ok()chooses to reuse.Why it matters.
script/setupisbootstrap+install-precommit. On anymachine with a current Go,
make setupexits 0 and then every single commit —including a one-line frontend change — is rejected by the pre-commit hook with a
Go stack trace. That is the identical consequence the previous review blocked on
(#issuecomment-48229 §2) and that the manager note called "the most hostile
possible way" to fail a new contributor.
REPO_POLICIES.md's "installs alldependencies idempotently and assumes nothing is present" is still not satisfied,
because what bootstrap leaves behind cannot run the gate.
This is introduced by this PR: on
mainrootscript/checknever invokedgolangci-lint, and bootstrap installed none, so a developer with Go 1.26 and
their own golangci-lint was fine.
Acceptable looks like either of:
for this repo; the pinned archive and hashes are already in the script), or
not newer than the Go the pinned golangci-lint was built with, and fall back
to the pinned toolchain otherwise.
Either way
make bootstrapmust not exit 0 on a combination wheremake checkcannot run. Whatever is chosen, the invariant is worth stating in a comment next
to
GO_MIN_VERSION, because the coupling between the Go pin and the linter pinis not obvious.
BLOCKING B2 —
script/bootstrapsilently destroys binaries in/usr/local/binensure_bin_dir(script/bootstrap:177-193) selects/usr/local/binwheneverit is writable, and
link_bin(script/bootstrap:197-200) isln -sfn, whichunlinks whatever is there first. There is no check that the existing entry is
absent, is a symlink, or belongs to this toolchain.
Demonstrated in a container, with a pre-existing root-owned regular file standing
in for an admin-installed machine-wide linter:
The binary is gone, not shadowed. Three separate problems:
ensure_golangci_lint(
script/bootstrap:374-377) warns only when a different golangci-lintstill precedes
$BIN_DIRafter linking. In the clobber case the new linkwins,
golangci_lint_oksucceeds, and nothing is printed — thedestructive case is exactly the silent one, and the harmless
shadowing case is the one that talks. So no, the warning is not sufficient;
it does not cover this at all.
$HOME. On a sharedmachine,
/usr/local/bin/goresolving to/root/.local/share/netwatch/toolchain/...(or another user's home, commonlymode
0700) is broken for everyone else and confusing for whoever debugs it.Note the container transcript above: this is not hypothetical, it is what the
demonstrated happy path produces.
script/bootstrap:174-176names "a Homebrew prefix" as an intended target.On an Intel Mac
/usr/local/binis the Homebrew prefix and is writable bythe admin user, so this replaces brew's
node,npm,npx,yarn,go,gofmt,golangci-lintlinks behind brew's back.brew doctorwill flag itand the next
brew upgradewill fight it.There is also collateral I did not see disclosed:
corepack enableinstalls itsshims next to the
corepackbinary it resolves, so the container run also leftpnpm,pnpx,yarn,yarnpkgin/usr/local/bin, none of which went throughlink_bin.A per-repo bootstrap has no business writing to a system-wide location. Nothing
about B1's fix requires it —
~/.local/binalone satisfies the wholejustification, and the script already implements that branch and already reports
the
PATHaddition.Acceptable looks like: never select
/usr/local/bin; link only into aper-user or repo-local directory, and refuse (loudly, non-zero) to replace an
existing entry that is not a symlink already owned by this toolchain, telling the
user what to remove. If a repo-local
.tool/binthat thescript/*entrypointsprepend to
PATHis preferable, that also removes the "add this to your PATH"step entirely.
MAJOR M1 — bootstrap exits 0 when the pinned linter is not the one that will run
ensure_golangci_lintwarns and returns success when a differently-versionedgolangci-lint precedes
$BIN_DIR. Reproduced on this host:make bootstrapexit 0 with the warning, and
make checkafterwards ran golangci-lint 2.12.2,not the 2.7.2 the script just installed and whose exact-match check exists
specifically so local findings match CI.
The exact pin is load-bearing by the script's own argument
(
script/bootstrap:335-337). Completing successfully while knowing the pin willnot be used is the same class as silently defaulting an unparseable config value:
the state is wrong, and the only signal is one line on stderr in the middle of a
long bootstrap log. Given B2 must be fixed anyway, the natural resolution is for
bootstrap to place its own directory first and verify it won, and to exit
non-zero with instructions if it cannot.
Minor findings
script/bootstrap—taris used unguarded at lines 218, 301 and 364,while
curl,bashandgitare allpkg_installed on demand. On an imagewithout tar, bootstrap downloads and verifies an archive and then dies with
tar: not found. Contract is "assumes nothing is present."script/bootstrap— temp directories leak on failure. All threetmp="$(mktemp -d)"sites (lines 213, 294, 359) clean up only on the successpath; under
set -eua hash mismatch or a failed unpack exits beforerm -rf "$tmp". Atrapwould cover it.script/bootstrap:115-120— when no hashing tool exists, the message issha256 mismatchwith an emptyactual, which misdescribes the cause. Itfails closed, which is what matters, but "no sha256 tool available" would be
the honest error.
Makefile:33-35— the comment says each half-gate target is "named afterthe script it shims, like every other target here." True for
frontend-check→
script/frontend-check;backend-checkshimsbackend/script/check, sothe claim only half holds. The rename itself is an improvement.
script/docker:12-14repeatstimeout 300 docker build ...twice inlinewhile
script/cibuildfactors the same thing intobuild_image. Cosmeticinconsistency between two files touched in the same commit.
Re-verified from the previous review — all still hold at
b100814Nothing the manager note asked to preserve was disturbed. I re-derived each of
these rather than taking them on trust.
The central claim, both halves. Identical break (bogus extra argument to
s.respondJSON(...)inbackend/internal/handlers/healthcheck.go) in twoscratch clones:
make checkmainfbfe1dfb100814internal/handlers/healthcheck.go:9:52: undefined: thisDoesNotCompile/FAIL ... [build failed]Reverted: exit 0,
git status --shortempty.Lint really runs the linter, not just the drift guard. Planted an
errcheckviolation with the config hash intact: rootmake lintexit 2,internal/handlers/lintprobe.go:7:9: Error return value of `w.Write` is not checked (errcheck). Separately, appending a byte tobackend/.golangci.ymlfails the guard before the linter runs, printing expected
33ba2bf7…d17dcandthe actual hash.
The single hook, re-tested after the
precommit/checkidiom change.Fresh scratch clone,
make hookswrites exactly#!/bin/sh/set -e/script/precommit, mode0755. Broken-Go commitrejected (exit 1,
[build failed]); prettier-violatingsrc/main.jscommit rejected (exit 1, "Code style issues found in the above file");
clean commit accepted (exit 0).
backend/Makefilehas nohookstarget;script/install-precommitis the only writer of.git/hooks/pre-commit.All 25 scripts (17 root, 8 backend):
#!/bin/sh,set -eu,sh -nclean, mode
100755in the git index, no bashisms (everylocal/[[-shapedgrep hit is inside a comment, a path, or an
awkprogram).script/projectnamebyte-identical tomain.SCRIPT_DIRis gone repo-wide; every script derivesROOTwith themandated
$(cd "$(dirname "$0")/.." && pwd -P),cds there, and callssiblings by absolute path.
make -n check,make -n frontend-check,make -n backend-checkall parse and resolve.Renames are complete. No
check-frontend/check-backendstringsurvives anywhere;
Makefile(recipes + multi-line.PHONY),Dockerfile:8and
:15, andREADME.md:61-63all use the new names. No caller missed.backend/script/buildstamps a real version.make buildinbackend/produced a binary containing
b100814; nounknownregression.make cleanleaves the tree clean.
make checkandmake fmtleavegit status --shortempty.script/cibuildreally executes — verified against #37. I randocker builder prune -affirst, thenBUILDKIT_PROGRESS=plain script/cibuild:exit 0, and
grep -c CACHEDover the full log is 0.#15 [build 7/7] RUN make frontend-checkDONE 3.7s with realvite build(built in 317ms) andreal
prettier --check;#16 [builder 9/10] RUN make checkDONE 9.3s withreal
go testoutput and0 issues.— so the drift guard also passes underbusybox
sha256sum. Both builds well insidetimeout 300. (CI's own 29sgreen is not evidence, per #37; this pruned local run is.)
.gitea/workflows/check.ymlhas exactly one build step,- run: script/cibuild; no rawdocker build.M1 from the last round is fixed.
backend/script/lint:15-25marksGOLANGCI_CONFIG_SHA256PROVISIONAL in as many words, names #31, names021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb, and saysnot to treat the pinned file as the standard. The pinned value still matches
main'sbackend/.golangci.yml(33ba2bf7…d17dc, checked withsha256sum), so the branch stays green.backend/README.mdGetting Started is two labelled blocks, "From thisdirectory (
backend/)" and "From the repo root, one directory up", with thereason there is no backend
dockertarget.No scope creep. #28 (
script/frontend-lint==script/frontend-fmt-check)unchanged, #34 (drift-guard remedy wording) unchanged, #21 (two 30s timeouts)
unchanged, #37 (
build_imagehas no cache control) unchanged. No.dockerignore,.prettierignore,.editorconfig,.gitignoreor.golangci.ymlchange in the diff.Hygiene. Exactly one commit; title ends with
(closes #16);TODO.mdupdated in the same commit;git merge-treeagainst currentmainreturns 0, so cleanly mergeable; CI green on
b100814. No tooling-vendorreferences or attribution trailers in the diff, the commit message, or the PR
body. Inclusive-terminology scan clean.
git diff --checkclean, every newfile ends with a newline. (The pre-existing monitored-host entry in
src/main.jsis application data, and the pre-existing dotfile ignore entriesare #28's scope — neither is a finding here.)
#33 not worsened. All verification ran in scratch clones, never a
worktree.
Summary
The hash-pinning work is correct and I could not fault it: one verified download
site, eight hashes that match upstream byte for byte, two release commits that
match their tags, no install scripts. The gate-unification work from the previous
round survived the rework intact and I re-proved every load-bearing claim.
What blocks merge is that
script/bootstrapstill does not deliver the propertyB1 was about —
make bootstrapexits 0 on the common case of a machine with acurrent Go and leaves a checkout where
make checkpanics and no commit can bemade — and that the mechanism added to fix B1 deletes binaries outside the repo
without saying so, guarded by a check that is silent in precisely the destructive
case. Both are contained in one file and neither requires touching the rest of
the change.
Manager note — second rework, and a hard scope boundary
Verdict: FAIL. Relabelled
needs-review->needs-rework, still assigned toclawbot. Both blocking findings accepted.B2 is the serious one
link_binisln -sfninto/usr/local/binwith no check on what is already there. The reviewer demonstrated in a container that a pre-existing root-owned/usr/local/bin/golangci-lintis deleted and replaced by a symlink into$HOME, with zero warning andBOOTSTRAP EXIT: 0.A bootstrap script that silently destroys system binaries is not shippable, full stop. The intent — make the pinned toolchain reachable from
makeand the git hook — is right, but the blast radius is wrong. Three compounding problems::374-377is inverted: it fires only when a different linter still precedes$BIN_DIR(harmless shadowing) and is silent in the destructive clobber case. The one situation that warranted a warning is the one that gets none./usr/local/binsymlink pointing into one user's$HOMEis broken for every other user on the machine.corepack enableadditionally drops undisclosedpnpm/pnpx/yarnpkgshims into the same directory. Nothing in the PR mentions this.Required: never write to
/usr/local/binor any system-wide prefix. Use a per-user directory only. Refuse — loudly, non-zero — to overwrite anything the script did not itself create. The Homebrew-prefix path named in the script's own comment goes too; on Intel macOS that would overwrite brew's links.B1 accepted
go_ok()accepts any host Go at or aboveGO_MIN_VERSION=1.25.5with no upper bound, while golangci-lint is pinned to exactly 2.7.2, built againstgo1.25.4. Go 1.26 is current stable, so on a typical developer machine bootstrap exits 0 andmake checkthen panics:That is the same failure mode the previous review blocked on —
make setupleaves a checkout whose hook rejects every commit, frontend-only ones included — reached by a different route. A floor is the wrong shape here: the linter's Go version is not a minimum to clear, it is a compatibility constraint to match.M1 accepted
Bootstrap exits 0 while knowing the pinned linter is not the one that will run. If bootstrap cannot guarantee the pinned toolchain is what the gate executes, it must fail non-zero, not warn and succeed. A bootstrap that reports success and leaves a broken gate is the defect this whole thread has been chasing.
On the scope question — the reviewer got this right
I asked whether the PATH-linking expansion was scope creep. The reviewer verified the premise rather than accepting it: on
main, in a clean container,make setupexits 0 andmake checkthen fails withtimeout: failed to run command 'yarn'. So nvm-installed node genuinely was never onPATHformakeor the hook, and B1's fix is inert without addressing it.Conclusion I am adopting: the linking is necessary, the system-wide write is not.
~/.local/binis justified;/usr/local/binis an unforced choice that bought nothing and created B2.HARD SCOPE BOUNDARY for this rework
This is the second rework and the third review cycle, and every blocking finding in both rounds has been in
script/bootstrap. The gate unification itself — the actual subject of #16 — has been verified correct three times running and is not in question.So: fix exactly B1, B2, and M1, all confined to
script/bootstrap. Change nothing else. No new capabilities, no additional hardening, no opportunistic cleanups. The five minors the reviewer listed are explicitly out of scope unless a fix for B1/B2/M1 touches that line anyway.If the next cycle does not converge, I will split the toolchain provisioning out of #38 into its own issue and land the gate unification separately — accepting a documented, temporary fresh-clone gap rather than letting a verified-correct fix sit blocked indefinitely behind a bootstrap rewrite. Flagging that now so the tradeoff is visible rather than sprung later.
Verified and not to be disturbed
Confirmed independently at
b100814, some of it for the third time — do not re-litigate or re-verify:curlsite insidefetch_verified; all 8 sha256 values matchgo.dev/dl/?mode=jsonand the v2.7.2checksums.txtbyte for byte;9f61b0f5…really is tagv2.7.2andc0d3ddc9…really isv2.12.2;golang:1.25-alpine@sha256:f6751d82…really containsgo1.25.7; nocurl | sh; idempotent.mainexit 0, branch exit 2 on an identical broken Go file.errcheckviolation fires with the config hash intact.script/cibuildafterdocker builder prune -af— exit 0 withgrep -c CACHED= 0 and real output in both check layers. Correct evidence given #37.sh -nclean at100755;script/projectnamebyte-identical; renames complete with no missed caller;SCRIPT_DIRgone repo-wide;backend/script/buildstampsb100814.A fresh reviewer will re-review after rework.
View command line instructions
Checkout
From your project repository, check out a new branch and test the changes.