build(lint): make gofmt a check, not a rewrite - #26
Conversation
The fmt recipe ran `go fmt ./...`, silently rewriting files instead of failing the lint gate on unformatted code (CG-GATE-01). Replace it with a gofmt -l check that fails on unformatted files, add fmt-fix for the old rewrite behavior, and format the one file HEAD was carrying unformatted (internal/tui/grouped_cursor_test.go).
| fmt: | ||
| #!/usr/bin/env bash | ||
| set -euo pipefail | ||
| unformatted=$(gofmt -l internal cmd) |
There was a problem hiding this comment.
This narrows the fmt gate to cmd/ and internal/, but the old go fmt ./... it replaces covered the whole module — including the 16 committed .go files under tests/integration/. Unformatted test code will now pass this gate silently, which is the same "gate reports clean while tree is dirty" failure this PR is fixing, just moved to tests/. It matters because lint runs in CI via just lint (test.yaml, release.yaml). Change to gofmt -l . (gofmt only inspects .go files, so bin//dist//out//completions/ are ignored) or gofmt -l cmd internal tests. This also aligns the checker with fmt-fix's go fmt ./..., which already formats tests/.
There was a problem hiding this comment.
Fixed. Gate now uses gofmt -l . (matches fmt-fix's go fmt ./... scope, and gofmt only touches .go files so bin/dist/out/completions are naturally excluded). Formatted the 8 flagged files under tests/integration/.
Re-verified: gofmt -l . clean at HEAD (b3b2d97), just lint exits 0 on the clean tree, and confirmed the negative case directly — injected an unformatted function into tests/integration/helpers.go, just lint failed with exit 1 (listing the file, pointing to fmt-fix), then reverted and reconfirmed clean. just test unit: 182/182 passed. Pushed to fix/camp-graph-lint at b3b2d97.
obey-agent
left a comment
There was a problem hiding this comment.
Staff Review — Verdict: REQUEST CHANGES
Overview
The PR correctly diagnoses the gate bug: the old fmt recipe ran go fmt ./..., a silent rewrite that mutated files during lint and therefore never failed the gate on unformatted code. Splitting into a checking fmt (gofmt -l + non-zero exit) and a rewriting fmt-fix is the right shape, and the bash-shebang implementation is portable and defensive.
Key Findings
- (major) Coverage regression. The old
go fmt ./...formatted every package in the module, includingtests/integration/(16 committed.gofiles). The newgofmt -l internal cmdonly walkscmd/andinternal/, so unformatted code undertests/passes the gate silently — the same failure mode this PR fixes, relocated totests/. Sincelintis wired into CI (test.yaml,release.yamlbothrun: just lint), the hole is load-bearing. Fix:gofmt -l .(gofmt only reads.gofiles) orgofmt -l cmd internal tests. This also removes the asymmetry wherefmt-fix(go fmt ./...) formatstests/butfmtnever verifies it.
What's Done Well
- Correct use of
gofmt -l+ explicit[[ -n "$unformatted" ]] -> exit 1(gofmt -l exits 0 even when it lists files, so this is the right way to fail the gate); empty-output correctly passes. #!/usr/bin/env bash+set -euo pipefailmakes the recipe robust across shells and bash-3.2/macOS compatible.- Preserving the rewrite path as
fmt-fixkeeps local fixing ergonomic; actionable "Run 'just fmt-fix'" message. - Reformatting
grouped_cursor_test.goso HEAD is clean, plus the documented negative test, is the right diligence.
Staff Standard
One real gap between "checks the whole module" and "checks two of three source dirs." It's a one-token change to close, and until it's closed the gate ships with a blind spot over committed test code. Everything else about the change is production-quality.
The fmt gate only checked internal and cmd, so unformatted files under tests/integration/ (8 of them) silently passed lint -- the same gate-doesn't-gate failure this branch is fixing, just relocated. gofmt -l . covers the whole module (gofmt only inspects .go files, so bin/dist/out/completions are unaffected) and matches fmt-fix's `go fmt ./...` scope. Formatted the 8 offending tests/integration files.
obey-agent
left a comment
There was a problem hiding this comment.
Staff Review — Verdict: COMMENT (merge-ready)
Overview
The follow-up commit resolves the prior review blocker. The formatting gate now checks the whole module with gofmt -l ., matching the scope of fmt-fix, and the previously missed integration test files are gofmt-clean.
Key Findings
- No remaining blocking findings. The earlier coverage gap over
tests/integration/is fixed.
What's Done Well
fmtnow actually gates formatting instead of rewriting files silently.fmt-fixpreserves the local rewrite workflow while keeping CI/check behavior strict.- The message printed on failure is actionable and points directly to
just fmt-fix.
Verification
gofmt -l .returned empty output.just lintjust test unit(182/182 tests passed)
Staff Standard
Yes. I would be comfortable submitting this as my own work now that the formatter check covers all committed Go packages in the module.
|
|
||
| # Rewrite Go code to be gofmt-formatted | ||
| fmt-fix: | ||
| go fmt ./... |
There was a problem hiding this comment.
fmt-fix uses go fmt ./..., whose scope is narrower than the fmt gate's gofmt -l .: go fmt ./... skips testdata/, vendor/, and ./_-prefixed dirs, while gofmt -l . inspects them. Harmless now (no .go files exist in those locations), but if a .go fixture lands under testdata/ unformatted, the gate would fail with no fix-recipe path. Consider gofmt -w . here for exact check/fix symmetry, or soften the commit message's "matches fmt-fix scope" claim. Non-blocking.
There was a problem hiding this comment.
Fixed in 476e368. fmt-fix now runs gofmt -w ., matching the fmt gate's gofmt -l . whole-module scope, so a future fixture under testdata//vendor//dot-prefixed dirs can't produce a red gate with no repair path. HEAD stays gofmt-clean (just fmt green).
obey-agent
left a comment
There was a problem hiding this comment.
Staff Re-Review (head b3b2d97) — Verdict: COMMENT — prior blocker FIXED
Overview
Delta since my prior review (9f7b612): the justfile gate widened from gofmt -l internal cmd to gofmt -l ., plus 8 tests/integration/*.go files reformatted (pure gofmt column-alignment, no behavioral change).
Prior finding — FIXED and verified end-to-end
The major coverage gap is closed: the gate is now gofmt -l . (whole module, recursive), so the tests/ tree is covered and its previously-unformatted files were reformatted. I reproduced the gate against the head tree: clean -> rc 0; injected an unformatted file -> rc 1 listing it; injected a parse-error file -> rc 2 (not swallowed). HEAD is gofmt-clean, so the newly-armed gate goes green rather than shipping self-red.
New finding (minor, inline)
fmt-fix uses go fmt ./..., whose scope is narrower than the fmt gate's gofmt -l . (go fmt ./... skips testdata/, vendor/, and ./_-prefixed dirs). Harmless today (no .go files in those locations), but a future testdata/*.go fixture could produce a red gate with no fmt-fix path. The commit message's "matches fmt-fix scope" claim is slightly off.
What's Done Well
Correct check-vs-rewrite separation; strict-mode shell with a clear failure message naming offending files and pointing at just fmt-fix; HEAD is actually formatted so the gate goes green.
Staff Standard
Gate reliability confirmed (non-zero on drift, zero on clean, non-zero on parse error). The one remaining item is a minor scope-symmetry nit. Merge-ready. Leaving the Approve for Lance.
The fmt gate checks gofmt -l . (whole module), but fmt-fix ran go fmt ./..., which skips testdata/, vendor/, and dot/underscore-prefixed dirs. A future fixture under those paths could fail the gate with no fmt-fix path to repair it. fmt-fix now rewrites with the same whole-module gofmt scope.
obey-agent
left a comment
There was a problem hiding this comment.
Staff Re-Review - Verdict: APPROVE
No blocking findings on the updated head 476e368.
The prior formatter-scope nit is resolved: fmt gates with gofmt -l ., and fmt-fix now rewrites the same whole-module scope with gofmt -w .. HEAD is gofmt-clean, so the gate no longer has a blind spot over directories that go fmt ./... would skip.
Verification performed:
git diff --check ddd235c66e61620ec5db4ab7cc108af39827955d...HEADgofmt -l .returned empty outputjust lintjust test unit(182/182 tests passed)
Summary
lintrecipe rango fmt ./...(a silent rewrite), so the gate never actually failed on unformatted code and HEAD was not gofmt-clean.fmtnow runsgofmt -l internal cmdand exits non-zero if anything is listed;fmt-fixpreserves the old rewrite behavior for local use.internal/tui/grouped_cursor_test.go) so HEAD is clean.Test plan
gofmt -l internal cmdreturns empty on this branchjust lintexits 0 on the clean treejust lintexits non-zero (fmt step fails before vet runs) when an unformatted file is present, then reverted the test injectionjust test unit— 182/182 tests passed