From db15111316efd36c3a9ebc5030ddce1d5f209ecc Mon Sep 17 00:00:00 2001 From: Niv Greenstein <88280771+NivGreenstein@users.noreply.github.com> Date: Wed, 23 Sep 2026 18:00:41 +0300 Subject: [PATCH 1/7] test(cmd): check the coordinates TestFormatParse parses A successful parse hit `continue` before the coordinate assertion, so the test only ever checked errors. go vet reports the assertion as unreachable (MAPCO-11498). Co-Authored-By: Claude Opus 5.5 --- cmd/shigola/cmd/tile_name_format_test.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/cmd/shigola/cmd/tile_name_format_test.go b/cmd/shigola/cmd/tile_name_format_test.go index df17693c..2c415c8c 100644 --- a/cmd/shigola/cmd/tile_name_format_test.go +++ b/cmd/shigola/cmd/tile_name_format_test.go @@ -160,12 +160,13 @@ func TestFormatParse(t *testing.T) { for k, tc := range testcases { z, x, y, err := tc.format.Parse(tc.input) - if errOk(tc.err, err) { - continue - } else { + if !errOk(tc.err, err) { t.Errorf("[%v] unexpected err, expected %v got %v", k, tc.err, err) continue } + if tc.err != nil { + continue + } if z != tc.z || x != tc.x || y != tc.y { t.Errorf("[%v] expected output (z:%v, x:%v, y:%v) got (z:%v, x:%v, y:%v)", k, tc.z, tc.x, tc.y, z, x, y) From 5d0b2c90b4acad0df437b65b12d2146b0e3a1ebc Mon Sep 17 00:00:00 2001 From: Niv Greenstein <88280771+NivGreenstein@users.noreply.github.com> Date: Wed, 23 Sep 2026 18:01:01 +0300 Subject: [PATCH 2/7] test(register): stop copying an atlas through TestMaps cases tcase carried an atlas.Atlas, which holds a mutex, and every case copied it by value. No case ever set it. go vet reports the copies (MAPCO-11498). Co-Authored-By: Claude Opus 5.5 --- cmd/internal/register/maps_test.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/cmd/internal/register/maps_test.go b/cmd/internal/register/maps_test.go index f8b71ff1..ae81ceff 100644 --- a/cmd/internal/register/maps_test.go +++ b/cmd/internal/register/maps_test.go @@ -14,7 +14,6 @@ import ( func TestMaps(t *testing.T) { type tcase struct { - atlas atlas.Atlas maps []provider.Map providers []dict.Dict expectedErr error @@ -36,7 +35,10 @@ func TestMaps(t *testing.T) { return } - err = register.Maps(&tc.atlas, tc.maps, providers) + // A fresh atlas per case, not one carried in tcase: an Atlas holds a + // mutex, and a tcase is copied by value into fn and by range. + var a atlas.Atlas + err = register.Maps(&a, tc.maps, providers) if !errors.Is(err, tc.expectedErr) { t.Errorf("invalid error, expected %v got %v", tc.expectedErr, err) } From 7292ee70a815e810592e96e8a6925110dbac2271 Mon Sep 17 00:00:00 2001 From: Niv Greenstein <88280771+NivGreenstein@users.noreply.github.com> Date: Wed, 23 Sep 2026 18:01:08 +0300 Subject: [PATCH 3/7] fix(cmd): buffer the channel the shutdown signal arrives on signal.Notify never blocks on a send, so with an unbuffered channel a SIGTERM that arrives before signalHandler is receiving is dropped and the process does not shut down. Flagged by go vet (MAPCO-11498). Co-Authored-By: Claude Opus 5.5 --- internal/cmd/main.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/internal/cmd/main.go b/internal/cmd/main.go index a30e286f..f2478d1d 100644 --- a/internal/cmd/main.go +++ b/internal/cmd/main.go @@ -157,7 +157,9 @@ func Signal() os.Signal { // can be passed in as well, if no list is passed os.Interrupt, and syscall.SIGTERM is // assumed. func NewContext(signals ...os.Signal) *contextType { - ch := make(chan os.Signal) + // Buffered, as signal.Notify requires: it never blocks on a send, so a signal + // arriving before signalHandler is receiving would otherwise be dropped. + ch := make(chan os.Signal, 1) if len(signals) == 0 { signal.Notify(ch, os.Interrupt, syscall.SIGTERM) } else { From 002d59532905f8b7d8b5533818ea36c5dce315dc Mon Sep 17 00:00:00 2001 From: Niv Greenstein <88280771+NivGreenstein@users.noreply.github.com> Date: Wed, 23 Sep 2026 18:01:14 +0300 Subject: [PATCH 4/7] refactor(prometheus): give Handler a pointer receiver like the rest A value receiver copied the whole observer, sync.Once fields included, on every call. Every other method already takes *observer, and only *observer satisfies the observer interfaces. Flagged by go vet (MAPCO-11498). Co-Authored-By: Claude Opus 5.5 --- observability/prometheus/prometheus.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/observability/prometheus/prometheus.go b/observability/prometheus/prometheus.go index b5899d90..78fd155b 100644 --- a/observability/prometheus/prometheus.go +++ b/observability/prometheus/prometheus.go @@ -133,8 +133,8 @@ func New(config dict.Dicter) (observability.Interface, error) { func (*observer) Name() string { return Name } -func (observer) Handler(string) http.Handler { return promhttp.Handler() } -func (obs *observer) Init() { obs.initCall.Do(obs.init) } +func (*observer) Handler(string) http.Handler { return promhttp.Handler() } +func (obs *observer) Init() { obs.initCall.Do(obs.init) } func (obs *observer) init() { obs.PublishBuildInfo() if obs == nil || obs.pushURL == "" { From 689c3a09261622309c11edbc77e17afa07e6f055 Mon Sep 17 00:00:00 2001 From: Niv Greenstein <88280771+NivGreenstein@users.noreply.github.com> Date: Wed, 23 Sep 2026 18:01:21 +0300 Subject: [PATCH 5/7] refactor(env): name the fields of dict.ErrKeyType literals An unkeyed literal of a type from another package breaks silently if its fields are reordered. go vet's composites check flags it (MAPCO-11498). Co-Authored-By: Claude Opus 5.5 --- internal/env/dict.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/internal/env/dict.go b/internal/env/dict.go index ec98bef9..6454886d 100644 --- a/internal/env/dict.go +++ b/internal/env/dict.go @@ -78,7 +78,7 @@ func (d Dict) StringSlice(key string) (v []string, err error) { var iv []interface{} if iv, ok = val.([]interface{}); !ok { // Could not convert to the generic type, so we don't have the correct thing. - return v, dict.ErrKeyType{key, val, reflect.TypeOf(iv)} + return v, dict.ErrKeyType{Key: key, Value: val, T: reflect.TypeOf(iv)} } v = make([]string, len(iv)) @@ -156,7 +156,7 @@ func (d Dict) BoolSlice(key string) (v []bool, err error) { var iv []interface{} if iv, ok = val.([]interface{}); !ok { // Could not convert to the generic type, so we don't have the correct thing. - return v, dict.ErrKeyType{key, val, reflect.TypeOf(iv)} + return v, dict.ErrKeyType{Key: key, Value: val, T: reflect.TypeOf(iv)} } v = make([]bool, len(iv)) @@ -232,7 +232,7 @@ func (d Dict) IntSlice(key string) (v []int, err error) { var iv []interface{} if iv, ok = val.([]interface{}); !ok { // Could not convert to the generic type, so we don't have the correct thing. - return v, dict.ErrKeyType{key, val, reflect.TypeOf(iv)} + return v, dict.ErrKeyType{Key: key, Value: val, T: reflect.TypeOf(iv)} } v = make([]int, len(iv)) for k := range iv { @@ -436,7 +436,7 @@ func (d Dict) MapSlice(key string) (r []dict.Dicter, err error) { arr, ok := v.([]map[string]interface{}) if !ok { - return r, dict.ErrKeyType{key, v, reflect.TypeOf(arr)} + return r, dict.ErrKeyType{Key: key, Value: v, T: reflect.TypeOf(arr)} } r = make([]dict.Dicter, len(arr)) From 777a390b8b675d78a4a6c9c7241db84384a511ba Mon Sep 17 00:00:00 2001 From: Niv Greenstein <88280771+NivGreenstein@users.noreply.github.com> Date: Wed, 23 Sep 2026 18:02:18 +0300 Subject: [PATCH 6/7] ci: fail on go vet findings and on gofmt -s violations A static checks job runs gofmt -s over every tracked Go file outside vendor/ and go vet over the tree. Both were clean only by convention, and vet was not: four kinds of finding had accumulated (MAPCO-11498). Co-Authored-By: Claude Opus 5.5 --- .github/workflows/on_pr_push.yml | 40 ++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/.github/workflows/on_pr_push.yml b/.github/workflows/on_pr_push.yml index 28d0b6e7..2217c5ee 100644 --- a/.github/workflows/on_pr_push.yml +++ b/.github/workflows/on_pr_push.yml @@ -203,6 +203,46 @@ jobs: if: always() run: docker compose down + # Its own job, so a formatting or vet failure is named in the checks list + # and arrives in seconds rather than after the fixture restore and the suite. + static: + name: Static checks + runs-on: ubuntu-22.04 + steps: + - name: Check out code + uses: actions/checkout@v4 + + - name: Set up Go + uses: actions/setup-go@v5 + with: + go-version-file: 'go.mod' + check-latest: true + + # vendor/ is committed and is not `gofmt -s` clean, so `gofmt -s -l .` + # would report hundreds of files nobody here owns. List the tracked Go + # files outside it instead -- a filter on gofmt's output would have to + # survive grep exiting 1 on the clean run under the runner's pipefail. + - name: Check formatting + run: | + unformatted=$(git ls-files -z '*.go' ':!:vendor/**' | xargs -0 gofmt -s -l) + if [ -n "$unformatted" ]; then + echo "::error::these files are not gofmt -s clean:" + echo "$unformatted" + exit 1 + fi + + # The default analyzers, nothing added: the tree was brought clean against + # exactly these (MAPCO-11498), so a finding here is new. vet skips vendor/ + # on its own; ./... never matches it. + # + # Twice, because vet only sees the files the build tags select. The second + # run sets every opt-out tag plus pprof, so the stub files that stand in + # for a compiled-out backend are vetted too, not only the default build. + - name: Vet + run: | + go vet -mod vendor ./... + go vet -mod vendor -tags 'noS3Cache noRedisCache noAzblobCache noGCSCache noGpkgProvider noPostgisProvider noPrometheusObserver pprof' ./... + govulncheck: name: Run govulncheck runs-on: ubuntu-22.04 From 5de821d6d31b7b7e8a7dfa171f41ad0ac9b995f4 Mon Sep 17 00:00:00 2001 From: Niv Greenstein <88280771+NivGreenstein@users.noreply.github.com> Date: Wed, 23 Sep 2026 18:02:36 +0300 Subject: [PATCH 7/7] docs(contributing): list gofmt -s and go vet as required checks Both now fail CI, so say so where contributors look, next to govulncheck (MAPCO-11498). Co-Authored-By: Claude Opus 5.5 --- CONTRIBUTING.md | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 9fe704ce..5fdcd1ef 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -281,8 +281,10 @@ Optional features compile out behind `noS3Cache`, `noRedisCache`, `noAzblobCache ## Code conventions -* **`gofmt -s`.** If running it produces changes in parts of the tree you are not working on, send - those in a separate pull request. +* **`gofmt -s` and `go vet` are required**, and CI enforces both (see [Required checks](#required-checks)). + Never run `gofmt -s -w .` at the root: it rewrites `vendor/`. Format the paths you changed, and if + that produces changes in parts of the tree you are not working on, send those in a separate pull + request. * **Error variables** take the form `var ErrErrorName = errors.New("provider: canceled")` — the text all lowercase, with no punctuation at the end. * **Table-driven subtests keyed by name**, with a `fn := func(tc tcase) func(*testing.T)` closure. @@ -324,13 +326,19 @@ CGO_ENABLED=0 go build -mod vendor ./... CGO_ENABLED=0 go test -run '^$' -mod vendor ./... # links every test binary, runs none ``` -Two more gates worth running before you push: +### Required checks + +CI fails a pull request on any of these, so run them before you push: ```bash -gofmt -s -l . | grep -v '^vendor/' # vendor/ is never -s clean; nothing else may appear +git ls-files -z '*.go' ':!:vendor/**' | xargs -0 gofmt -s -l # vendor/ is never -s clean; must print nothing +go vet -mod vendor ./... # the default analyzers; the tree is clean against them govulncheck ./... ``` +The tree is clean against `gofmt -s` and `go vet` today, so a finding from either is one your change +introduced. Fix it rather than working around the check. + ### Opt-in suites Provider- and backend-specific tests **skip unless you opt in** (`internal/ttools.ShouldSkip`), so a