From 34e1755ef8eab79ecddb9d0333f9dadabccb3b4c Mon Sep 17 00:00:00 2001 From: Arturo Peroni Date: Wed, 3 Jun 2026 11:27:58 +0200 Subject: [PATCH] fix: address Bugbot findings from PR #22 review 1. target_size: emit [height, width] (was [width, height]) to match the ingest.v1 schema + ingestor; only affected non-square datasets. Added TestBuild_TargetSize_EmittedHeightWidth (non-square) regression test. 2. Makefile: make ci lint now runs the same standalone tools as CI (errcheck + ineffassign + misspell) instead of golangci-lint, restoring the local==CI invariant while golangci-lint-action stays disabled (#6); golangci-lint moved to make lint-full. Co-Authored-By: Claude Opus 4.8 (1M context) --- Makefile | 13 +++++++++++++ internal/push/spec.go | 14 +++++++++++--- internal/push/spec_test.go | 33 +++++++++++++++++++++++++++++++++ 3 files changed, 57 insertions(+), 3 deletions(-) diff --git a/Makefile b/Makefile index 0749fa4..bcf7328 100644 --- a/Makefile +++ b/Makefile @@ -46,8 +46,21 @@ test: test-integration: $(GO) test -tags integration -count=1 -timeout 10m -v ./test/integration/... +# Lint set matched to .github/workflows/build.yml's lint job: errcheck + +# ineffassign + misspell (gofmt -s is `fmt-check`, go vet is `vet`). +# golangci-lint-action is disabled in CI pending tracebloc/cli#6 — so +# until it's re-enabled there, `make ci` runs the SAME standalone tools +# the CI lint job runs, keeping the "make ci green => CI green" invariant +# this Makefile exists to protect. `make lint-full` keeps golangci-lint +# available for a richer local pass. .PHONY: lint lint: + $(GO) run github.com/kisielk/errcheck@latest ./... + $(GO) run github.com/gordonklaus/ineffassign@latest ./... + $(GO) run github.com/client9/misspell/cmd/misspell@latest -error . + +.PHONY: lint-full +lint-full: @command -v $(GOLANGCI_LINT) >/dev/null 2>&1 || { \ echo "==> $(GOLANGCI_LINT) not on PATH"; \ echo " install via: brew install golangci-lint"; \ diff --git a/internal/push/spec.go b/internal/push/spec.go index 2133976..a28d7e8 100644 --- a/internal/push/spec.go +++ b/internal/push/spec.go @@ -134,7 +134,8 @@ type SpecArgs struct { // image_classification cares about. LabelColumn string - // TargetSize, when len==2, pins the image resolution as [W, H]. + // TargetSize, when len==2, holds the resolution as [W, H] — the + // order produced by --target-size "WxH" and by DetectImageSize. // The ingestor's image_classification default is 512x512 and it // VALIDATES (it does not resize), so a dataset whose images don't // match the default hard-fails. Setting this emits @@ -142,6 +143,10 @@ type SpecArgs struct { // resolution wins. Empty (len 0) ⇒ omit and let the ingestor // default apply. Populated by the CLI from --target-size or by // auto-detecting the first image. (Image categories only.) + // + // NOTE: stored [W, H] here, but EMITTED as [height, width] by + // Build — that's the order the ingest.v1 schema documents and the + // ingestor reads. buildImage does the swap. (Bugbot, PR #22.) TargetSize []int // Schema is the column→SQL-type map for tabular / time-series @@ -253,7 +258,9 @@ func (a SpecArgs) buildImage(spec map[string]any, prefix string) { if a.Category == "keypoint_detection" { if len(a.TargetSize) == 2 { - spec["target_size"] = []int{a.TargetSize[0], a.TargetSize[1]} + // Schema documents target_size as [height, width]; TargetSize + // is stored [W, H], so swap on emit. (Bugbot, PR #22.) + spec["target_size"] = []int{a.TargetSize[1], a.TargetSize[0]} } if a.NumberOfKeypoints > 0 { spec["number_of_keypoints"] = a.NumberOfKeypoints @@ -264,7 +271,8 @@ func (a SpecArgs) buildImage(spec map[string]any, prefix string) { if len(a.TargetSize) == 2 { spec["spec"] = map[string]any{ "file_options": map[string]any{ - "target_size": []int{a.TargetSize[0], a.TargetSize[1]}, + // [height, width] per the schema; TargetSize is [W, H]. + "target_size": []int{a.TargetSize[1], a.TargetSize[0]}, }, } } diff --git a/internal/push/spec_test.go b/internal/push/spec_test.go index 2167008..09c6a4c 100644 --- a/internal/push/spec_test.go +++ b/internal/push/spec_test.go @@ -197,6 +197,39 @@ func TestBuild_NoTargetSize_OmitsSpecBlock(t *testing.T) { } } +// TestBuild_TargetSize_EmittedHeightWidth locks the [height, width] +// emit order (Bugbot, PR #22). TargetSize is stored [W, H] but the +// ingest.v1 schema + ingestor read target_size as [height, width]. +// Square sizes can't catch a swap, so this uses a NON-square +// resolution: [W, H] = [640, 480] must emit [480, 640]. +func TestBuild_TargetSize_EmittedHeightWidth(t *testing.T) { + // image_classification → under spec.file_options + icSpec := SpecArgs{ + Table: "t", Category: "image_classification", Intent: "train", + LabelColumn: "label", TargetSize: []int{640, 480}, // [W, H] + }.Build() + sb, ok := icSpec["spec"].(map[string]any) + if !ok { + t.Fatalf("image: no spec block: %#v", icSpec["spec"]) + } + fo, ok := sb["file_options"].(map[string]any) + if !ok { + t.Fatalf("image: no file_options: %#v", sb["file_options"]) + } + if ts, ok := fo["target_size"].([]int); !ok || len(ts) != 2 || ts[0] != 480 || ts[1] != 640 { + t.Errorf("image target_size = %#v, want [480 640] (height, width)", fo["target_size"]) + } + + // keypoint_detection → top-level + kpSpec := SpecArgs{ + Table: "t", Category: "keypoint_detection", Intent: "train", + LabelColumn: "image_label", TargetSize: []int{640, 480}, NumberOfKeypoints: 9, + }.Build() + if ts, ok := kpSpec["target_size"].([]int); !ok || len(ts) != 2 || ts[0] != 480 || ts[1] != 640 { + t.Errorf("keypoint top-level target_size = %#v, want [480 640] (height, width)", kpSpec["target_size"]) + } +} + // TestBuild_Tabular_PassesSchema pins the tabular Build branch: it // emits schema-valid specs for the three label shapes — a string // label (tabular_classification), an object label+policy