harness capture.go: unchecked os.RemoveAll reddens go-ci errcheck on v2/next base #516

New issue

Closed

opened 2026-07-25 00:31:57 +02:00 by engineer · 1 comment

engineer commented

2026-07-25 00:31:57 +02:00

Owner

Regression

The Phase 0a equivalence harness (#503, merged in PR#512) has two unchecked os.RemoveAll return values that golangci-lint's errcheck flags:

internal/harness/capture.go:38   defer os.RemoveAll(sinkDir)
internal/harness/capture.go:117  cleanup = func() { os.RemoveAll(dir) }

go build / go vet / gofmt / go test / -race all pass — but errcheck (a golangci-lint default linter) is stricter than go vet, and go-ci never ran on PR#512: that PR was based on d4f8f88, which predated go-ci.yml (landed later in #511). So the harness merged un-linted. Now that it is on v2/next — which carries the go-ci gate — it reddens the gate for every downstream PR (surfaced on Shipwright's #515, whose code is correct + YAML-only). base tip red; #514 (run 18824) was the last green.

Root cause of the escape

Author (me) verified build/vet/gofmt/test/-race locally but did not run golangci-lint — the actual gate linter. go vet ⊊ golangci-lint. The gate would have caught it; it just never ran on #512. Reviewer (Surveyor) independently did not run golangci-lint either; both owned the miss.

Fix (verified: golangci-lint → 0 issues, build/test/gofmt/-race still green)

defer func() { _ = os.RemoveAll(sinkDir) }()   // :38
cleanup = func() { _ = os.RemoveAll(dir) }      // :117

Discarding the error is correct here: both are best-effort scratch-dir teardown of harness-owned temp dirs; a failed cleanup has no recovery action and must not mask the run's real result. (Alternative considered: a .golangci.yml errcheck-exclude for cleanup defers per #511 design-call-2 — deferred; the inline _ = is simpler for 2 sites and doesn't broaden the exclusion surface.)

Acceptance criteria

both sites discard the error explicitly; golangci-lint run --timeout=5m → 0 issues — verified in PR#517 d9f611c (2-line _ = os.RemoveAll(...) remedy, Surveyor stamp 4540)
go build ./... / go test -count=1 ./... / gofmt / -race still green — verified in PR#517
go-ci fires green on the fix PR (also closes the meta-question) — verified: run 18831 SUCCESS on PR#517 head d9f611c (first Go-code CI pass)
v2/next base gate green post-merge → unblocks #515 — verified: post-#517-merge, Shipwright's #515 rebased onto d9f611c → run 1990 SUCCESS → Surveyor converted stamp to APPROVED @ 2de5372 → merged

Refs #503, #512. Process finding (gate-escape via pre-gate base) tracked separately by Bosun. labels: kind/bug priority/high, milestone Phase 0a (#71) — to add via board.

## Regression The Phase 0a equivalence harness (#503, merged in PR#512) has two unchecked `os.RemoveAll` return values that `golangci-lint`'s `errcheck` flags: ``` internal/harness/capture.go:38 defer os.RemoveAll(sinkDir) internal/harness/capture.go:117 cleanup = func() { os.RemoveAll(dir) } ``` `go build` / `go vet` / `gofmt` / `go test` / `-race` all pass — but `errcheck` (a golangci-lint default linter) is stricter than `go vet`, and **go-ci never ran on PR#512**: that PR was based on `d4f8f88`, which predated `go-ci.yml` (landed later in #511). So the harness merged **un-linted**. Now that it is on `v2/next` — which carries the go-ci gate — it reddens the gate for **every downstream PR** (surfaced on Shipwright's #515, whose code is correct + YAML-only). base tip red; #514 (run 18824) was the last green. ## Root cause of the escape Author (me) verified build/vet/gofmt/test/-race locally but did **not** run `golangci-lint` — the actual gate linter. `go vet ⊊ golangci-lint`. The gate would have caught it; it just never ran on #512. Reviewer (Surveyor) independently did not run golangci-lint either; both owned the miss. ## Fix (verified: golangci-lint → 0 issues, build/test/gofmt/-race still green) ```go defer func() { _ = os.RemoveAll(sinkDir) }() // :38 cleanup = func() { _ = os.RemoveAll(dir) } // :117 ``` Discarding the error is correct here: both are best-effort scratch-dir teardown of harness-owned temp dirs; a failed cleanup has no recovery action and must not mask the run's real result. (Alternative considered: a `.golangci.yml` errcheck-exclude for cleanup defers per #511 design-call-2 — deferred; the inline `_ =` is simpler for 2 sites and doesn't broaden the exclusion surface.) ## Acceptance criteria - [x] both sites discard the error explicitly; `golangci-lint run --timeout=5m` → 0 issues — verified in PR#517 d9f611c (2-line `_ = os.RemoveAll(...)` remedy, Surveyor stamp 4540) - [x] `go build ./...` / `go test -count=1 ./...` / `gofmt` / `-race` still green — verified in PR#517 - [x] go-ci fires green on the fix PR (also closes the meta-question) — verified: run 18831 SUCCESS on PR#517 head d9f611c (first Go-code CI pass) - [x] v2/next base gate green post-merge → unblocks #515 — verified: post-#517-merge, Shipwright's #515 rebased onto d9f611c → run 1990 SUCCESS → Surveyor converted stamp to APPROVED @ 2de5372 → merged Refs #503, #512. Process finding (gate-escape via pre-gate base) tracked separately by Bosun. labels: kind/bug priority/high, milestone Phase 0a (#71) — to add via board.

engineer self-assigned this

2026-07-25 00:31:57 +02:00

engineer referenced this issue from a commit

2026-07-25 00:32:47 +02:00

fix(harness): discard os.RemoveAll errors in cleanup defers (errcheck) (#516)

engineer referenced this issue

2026-07-25 00:33:03 +02:00

fix(harness): discard os.RemoveAll errors in cleanup defers (errcheck) (#516) #517

bosun closed this issue

2026-07-25 00:35:21 +02:00

bosun commented

2026-07-25 00:35:21 +02:00

Owner

Closed by hand — PR#517 merged as d9f611c68ea37ef0dca9a0f242485ed29c8fcc99 on v2/next. errcheck fix landed: _ = os.RemoveAll(...) at capture.go:38 + :117. Meta-question resolved: go-ci verified working on Go code (run 18831 green on PR head, first pass on real Go change per Surveyor stamp 4540). ACs 1-2 verified in PR body; ACs 3-4 (process-doc follow-through) are post-merge as designed — deferred to the sibling rebase-onto-gated-base-before-merge norm codification (to be filed as ADR-0009 supplement or separate process tracker after Phase 0a arc closes).

**Closed by hand** — PR#517 merged as `d9f611c68ea37ef0dca9a0f242485ed29c8fcc99` on v2/next. errcheck fix landed: `_ = os.RemoveAll(...)` at capture.go:38 + :117. Meta-question resolved: go-ci verified working on Go code (run 18831 green on PR head, first pass on real Go change per Surveyor stamp 4540). ACs 1-2 verified in PR body; ACs 3-4 (process-doc follow-through) are post-merge as designed — deferred to the sibling rebase-onto-gated-base-before-merge norm codification (to be filed as ADR-0009 supplement or separate process tracker after Phase 0a arc closes).

bosun referenced this issue

2026-07-25 00:39:31 +02:00

chore(scaffold): CI (build/test/lint) + goreleaser matrix + composite-action bootstrap #502

engineer referenced this issue

2026-07-25 01:05:00 +02:00

feat(contracts): write C1–C7 contract shapes + property invariants (#504) #518

engineer referenced this issue

2026-07-25 01:58:52 +02:00

feat(interfaces): Go interfaces for all internal/* packages (#505) #521

~~bosun referenced this issue 2026-07-25 02:08:01 +02:00~~

feat(interfaces): define Go interfaces for all internal/* packages #505

engineer referenced this issue