From 0b1bc3676f4c17806b4a7c5f510ebadf3e733fd5 Mon Sep 17 00:00:00 2001 From: Andreas Trawoeger Date: Sat, 24 Oct 2026 17:11:46 +0200 Subject: [PATCH 1/2] test(task): close the pipes the skipped-step CLI test opens, and drain them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The pr-validator's PASS on PR #45 carried two non-blocking observations, and a finding raised on a PR is never filed away. The first is a real defect in the test PR #45 added: `errW, _, err := os.Pipe()` binds the READ end to errW and drops the write end, so os.Stderr was pointed at a read end and the write end leaked. Both ends are now kept, the read end is drained after the write end is closed (an undrained pipe blocks the next writer), and the report is asserted to belong on stdout. The JSON test's read end is closed too. The second observation was about the PR BODY's R5 sweep: its first claim key (`task %s: %d step(s), %d failed"`) was vacuous — the old code ended the literal with `failed\n`, so the key could never match and the grep proved nothing. The corrected sweep is in the successor PR's body, with the before-tree match as its positive control; the gap in pr_body_lint.py (a vacuous claim key passes the no-match rule) is carried as evidence on opencharly/marketplace#473. The tier is `analysed on a live system`, not `fully tested and validated`: this repo's own gate (build, test, vet, gofmt, `charly box validate`, and the live gate that compiles the candy into a real charly binary) passed on the committed tree, and the changed path — the test — ran live. The FULL assembled bed is opencharly/charly's, and by the producer-first order it has not run on this head yet; claiming a gate that has not run would be claiming more than the evidence carries. Assisted-by: Claude Code minimax/minimax-m3.1-flash:cloud (analysed on a live system) --- candy/plugin-task/skipped_test.go | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/candy/plugin-task/skipped_test.go b/candy/plugin-task/skipped_test.go index f9e8819..42e4168 100644 --- a/candy/plugin-task/skipped_test.go +++ b/candy/plugin-task/skipped_test.go @@ -169,6 +169,7 @@ func TestPrintResultsJSON_CarriesTheSkippedCount(t *testing.T) { if _, err := buf.ReadFrom(r); err != nil { t.Fatal(err) } + _ = r.Close() var decoded []struct { Failed int `json:"failed"` Skipped int `json:"skipped"` @@ -205,7 +206,11 @@ func TestRunTaskCLI_SkippedStepExitsZero(t *testing.T) { if err != nil { t.Fatal(err) } - errW, _, err := os.Pipe() + // The stderr pipe gets BOTH ends: os.Pipe returns (read, write), and a pipe + // whose write end is dropped can never be drained — the read end would block a + // later writer forever and the descriptor would leak. The read end is drained + // below, after the write end is closed. + errR, errW, err := os.Pipe() if err != nil { t.Fatal(err) } @@ -218,7 +223,13 @@ func TestRunTaskCLI_SkippedStepExitsZero(t *testing.T) { t.Fatalf("a task whose only step ran nothing must exit 0, got %v", runErr) } report, _ := io.ReadAll(outR) + _ = outR.Close() + stderrReport, _ := io.ReadAll(errR) + _ = errR.Close() if !strings.Contains(string(report), "0 failed, 1 skipped") { t.Fatalf("the report must name the skipped count, got:\n%s", report) } + if stderrReport != nil && len(stderrReport) > 0 { + t.Fatalf("the report belongs on stdout, not stderr, got:\n%s", stderrReport) + } } From ba44d5c120cd7adc0c0bcb7b9099d5cbe8351f56 Mon Sep 17 00:00:00 2001 From: Andreas Trawoeger Date: Sat, 24 Oct 2026 17:34:11 +0200 Subject: [PATCH 2/2] test(task): one capture shape for the whole package, and the linter in the gate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The CI `go` job on this branch went red on the commit it was given: candy/plugin-task/skipped_test.go:232:5: S1009: should omit nil check; len() for nil slices is defined as zero (staticcheck) RCA. The mechanism is a dead condition: `io.ReadAll` never returns nil, so `stderrReport != nil && len(stderrReport) > 0` says what `len(stderrReport) > 0` already said. The line is mine — it is the stderr assertion this branch added — so the finding is blocking, and the `validate / arm` job failed behind it (its step is "fail on any check that did not pass", so auto-merge never armed). The MISSED CONTROL is the honest part. CI's `go` job runs gofmt, golangci-lint, go vet and go test; the gate I ran before pushing — and the repo's own documented gate list in AGENTS.md — ran the build, the tests, gofmt and the vet, but not the linter. The lint step lived only in CI, so a finding CI would raise was invisible until CI raised it. The root fix goes into that control as well as into the code: the documented local gate now names CI's `go` job steps — the linter and the vet — and the linter runs under the toolchain the module's own go.mod declares, because golangci-lint v2.13.2 cannot read the export data of a newer GOROOT. The linter's pinned version stays CI's, in `.github/workflows/ci.yml` — one source of truth. The validator's own review of this branch raised three more notes on the same capture, and a finding raised on a PR is never filed away: 1. the dead `!= nil` check — the finding above; 2. the globals were reassigned without a restore on every path, so a panic in the run under test would leave the NEXT test in the binary writing its output into a pipe nobody reads; 3. an early `t.Fatalf` leaked the pipes, because there were no defers. All three are fixed, and the fix is ONE shape rather than four: the capture was written four times in this package (skipped_test.go twice, cli_test.go once, plugin_test.go once), and the second copy was already wrong — that is how the leaked write end in PR #45 happened. `captureStreams` is now the only capture: it keeps both ends of both pipes, closes the write ends before the drain (io.ReadAll returns EOF only when every write end is closed, and an undrained pipe blocks the next writer), and restores the globals on every exit path including a panic. The four call sites use it, and the inline copies are deleted in the same cutover. The stdin captures in runner_test.go are a different behavior (they capture os.Stdin, not the reporter's output) and are untouched. The control proves the shape is load-bearing: swapping the two pipes in the helper turns FOUR tests red — the skipped-step CLI test, the JSON report test, the --output mode test and the reporter's end-to-end arm. The tier stays `analysed on a live system`: this repo's own gate passes on the committed tree and the changed path ran live, but the full assembled bed is opencharly/charly's, and it has not run on this head yet. Assisted-by: Claude Code minimax/minimax-m3.1-flash:cloud (analysed on a live system) --- AGENTS.md | 7 ++++ candy/plugin-task/capture_test.go | 51 ++++++++++++++++++++++++++++ candy/plugin-task/cli_test.go | 26 ++++---------- candy/plugin-task/plugin_test.go | 26 +++++--------- candy/plugin-task/skipped_test.go | 56 +++++++------------------------ 5 files changed, 86 insertions(+), 80 deletions(-) create mode 100644 candy/plugin-task/capture_test.go diff --git a/AGENTS.md b/AGENTS.md index e03b907..9d0185d 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -48,6 +48,13 @@ when one is authored, add it here. - `cd candy/plugin-task && go test ./...` — the dependency closure, CLI parsing, validation, and incremental-semantics tests. - `cd candy/plugin-task && gofmt -l .` — formatting (empty output passes). +- `cd candy/plugin-task && go vet ./...` — the vet pass. +- `cd candy/plugin-task && GOTOOLCHAIN=go$(go list -m -f '{{.GoVersion}}') golangci-lint run` + — the linter, under the toolchain this module's `go.mod` declares: golangci-lint + cannot read the export data of a newer GOROOT, so the toolchain must be the + module's own, and the linter's version is the one CI pins in + `.github/workflows/ci.yml` (one source of truth — read it there). The local gate + runs what CI's `go` job runs, so a finding CI would raise is raised locally first. - `charly box validate` at the repo root — the structural check (the candy + `plugin:` block, CUE schema). - `scripts/gate-task.sh` at the repo root — the live gate: it clones charly at diff --git a/candy/plugin-task/capture_test.go b/candy/plugin-task/capture_test.go new file mode 100644 index 0000000..144ff39 --- /dev/null +++ b/candy/plugin-task/capture_test.go @@ -0,0 +1,51 @@ +package task + +import ( + "io" + "os" + "testing" +) + +// captureStreams runs fn with os.Stdout and os.Stderr pointed at pipes and returns +// what each stream received. +// +// ONE shape for the whole package: a test that asserts on what the reporter wrote +// must not re-implement the capture, because a capture written twice is a capture +// written once correctly and once wrongly. This one is written correctly: +// +// - BOTH ends of BOTH pipes are kept. os.Pipe returns (read, write); a pipe whose +// write end is dropped can never be drained, so the read end would block a later +// writer forever and the descriptor would leak. +// - The write ends are closed BEFORE the drain. io.ReadAll returns EOF only when +// every write end is closed, and a pipe left undrained blocks the next writer. +// - The globals go back on every exit path, including a panic in fn — which the +// testing package recovers and then runs the NEXT test with. Without the +// restore, that test would write its output into a pipe nobody reads. +// - The restore happens before the caller asserts, so a failure message is never +// written to a closed pipe (and lost). +func captureStreams(t *testing.T, fn func()) (stdout, stderr string) { + t.Helper() + outR, outW, err := os.Pipe() + if err != nil { + t.Fatal(err) + } + errR, errW, err := os.Pipe() + if err != nil { + _ = outR.Close() + _ = outW.Close() + t.Fatal(err) + } + oldOut, oldErr := os.Stdout, os.Stderr + os.Stdout, os.Stderr = outW, errW + defer func() { + os.Stdout, os.Stderr = oldOut, oldErr + }() + fn() + _ = outW.Close() + _ = errW.Close() + out, _ := io.ReadAll(outR) + _ = outR.Close() + errOut, _ := io.ReadAll(errR) + _ = errR.Close() + return string(out), string(errOut) +} diff --git a/candy/plugin-task/cli_test.go b/candy/plugin-task/cli_test.go index 3b7c4c3..b3666a0 100644 --- a/candy/plugin-task/cli_test.go +++ b/candy/plugin-task/cli_test.go @@ -2,8 +2,6 @@ package task import ( "context" - "io" - "os" "strings" "testing" @@ -44,29 +42,17 @@ func TestRunTaskCLI_OutputMode(t *testing.T) { } defer func() { entityCache = oldCache }() - oldOut, oldErr := os.Stdout, os.Stderr - outR, outW, err := os.Pipe() - if err != nil { - t.Fatal(err) - } - errR, errW, err := os.Pipe() - if err != nil { - t.Fatal(err) - } - os.Stdout, os.Stderr = outW, errW - runErr := runTaskCLI(context.Background(), nil, []string{"capture", "--output"}) - _ = outW.Close() - _ = errW.Close() - os.Stdout, os.Stderr = oldOut, oldErr + var runErr error + stdout, stderr := captureStreams(t, func() { + runErr = runTaskCLI(context.Background(), nil, []string{"capture", "--output"}) + }) if runErr != nil { t.Fatalf("runTaskCLI --output: %v", runErr) } - stdout, _ := io.ReadAll(outR) - stderr, _ := io.ReadAll(errR) - if string(stdout) != "CAPTURED-RAW-VALUE\n" { + if stdout != "CAPTURED-RAW-VALUE\n" { t.Fatalf("stdout must carry ONLY the raw captured value + newline, got %q", stdout) } - if !strings.Contains(string(stderr), "task capture:") { + if !strings.Contains(stderr, "task capture:") { t.Fatalf("the human report must be on stderr, got %q", stderr) } } diff --git a/candy/plugin-task/plugin_test.go b/candy/plugin-task/plugin_test.go index 2dac3f8..3962117 100644 --- a/candy/plugin-task/plugin_test.go +++ b/candy/plugin-task/plugin_test.go @@ -3,7 +3,6 @@ package task import ( "context" "encoding/json" - "io" "os" "path/filepath" "strings" @@ -396,23 +395,16 @@ func TestFormatStepLine_RendersStepMessage(t *testing.T) { // asserts the step message reaches it — the end-to-end arm the pure renderer test // cannot cover (a reporter that stopped calling the renderer would pass that one). func TestPrintResultText_EmitsStepMessage(t *testing.T) { - old := os.Stdout - r, w, err := os.Pipe() - if err != nil { - t.Fatal(err) - } - os.Stdout = w - printResultText(outWriter(), &runResult{ - Name: "map", Status: "ran", - Steps: []spec.StepResult{{ - Keyword: "run", Text: "print the submodule map", - Result: spec.CheckResult{Status: spec.StatusPass, Message: "MAP-TABLE-MARKER"}, - }}, + out, _ := captureStreams(t, func() { + printResultText(outWriter(), &runResult{ + Name: "map", Status: "ran", + Steps: []spec.StepResult{{ + Keyword: "run", Text: "print the submodule map", + Result: spec.CheckResult{Status: spec.StatusPass, Message: "MAP-TABLE-MARKER"}, + }}, + }) }) - _ = w.Close() - os.Stdout = old - out, _ := io.ReadAll(r) - if !strings.Contains(string(out), "MAP-TABLE-MARKER") { + if !strings.Contains(out, "MAP-TABLE-MARKER") { t.Fatalf("printResultText must emit the step message, got:\n%s", out) } } diff --git a/candy/plugin-task/skipped_test.go b/candy/plugin-task/skipped_test.go index 42e4168..f10e72b 100644 --- a/candy/plugin-task/skipped_test.go +++ b/candy/plugin-task/skipped_test.go @@ -4,8 +4,6 @@ import ( "bytes" "context" "encoding/json" - "io" - "os" "strings" "testing" @@ -153,32 +151,22 @@ func TestPrintResultsJSON_CarriesTheSkippedCount(t *testing.T) { Skipped: 1, Steps: []spec.StepResult{{Keyword: "run", Text: "the step that ran nothing", Result: spec.CheckResult{Status: spec.StatusSkip}}}, }} - oldOut := os.Stdout - r, w, err := os.Pipe() - if err != nil { - t.Fatal(err) - } - os.Stdout = w - jerr := printResultsJSON(results) - _ = w.Close() - os.Stdout = oldOut + var jerr error + out, _ := captureStreams(t, func() { + jerr = printResultsJSON(results) + }) if jerr != nil { t.Fatalf("printResultsJSON: %v", jerr) } - var buf bytes.Buffer - if _, err := buf.ReadFrom(r); err != nil { - t.Fatal(err) - } - _ = r.Close() var decoded []struct { Failed int `json:"failed"` Skipped int `json:"skipped"` } - if err := json.Unmarshal(buf.Bytes(), &decoded); err != nil { - t.Fatalf("the JSON report must decode: %v (%s)", err, buf.String()) + if err := json.Unmarshal([]byte(out), &decoded); err != nil { + t.Fatalf("the JSON report must decode: %v (%s)", err, out) } if len(decoded) != 1 || decoded[0].Skipped != 1 { - t.Fatalf("the JSON report must carry the skipped count, got %s", buf.String()) + t.Fatalf("the JSON report must carry the skipped count, got %s", out) } } @@ -201,35 +189,17 @@ func TestRunTaskCLI_SkippedStepExitsZero(t *testing.T) { } defer func() { entityCache = oldCache }() - oldOut, oldErr := os.Stdout, os.Stderr - outR, outW, err := os.Pipe() - if err != nil { - t.Fatal(err) - } - // The stderr pipe gets BOTH ends: os.Pipe returns (read, write), and a pipe - // whose write end is dropped can never be drained — the read end would block a - // later writer forever and the descriptor would leak. The read end is drained - // below, after the write end is closed. - errR, errW, err := os.Pipe() - if err != nil { - t.Fatal(err) - } - os.Stdout, os.Stderr = outW, errW - runErr := runTaskCLI(context.Background(), nil, []string{"mixed"}) - _ = outW.Close() - _ = errW.Close() - os.Stdout, os.Stderr = oldOut, oldErr + var runErr error + report, stderrReport := captureStreams(t, func() { + runErr = runTaskCLI(context.Background(), nil, []string{"mixed"}) + }) if runErr != nil { t.Fatalf("a task whose only step ran nothing must exit 0, got %v", runErr) } - report, _ := io.ReadAll(outR) - _ = outR.Close() - stderrReport, _ := io.ReadAll(errR) - _ = errR.Close() - if !strings.Contains(string(report), "0 failed, 1 skipped") { + if !strings.Contains(report, "0 failed, 1 skipped") { t.Fatalf("the report must name the skipped count, got:\n%s", report) } - if stderrReport != nil && len(stderrReport) > 0 { + if stderrReport != "" { t.Fatalf("the report belongs on stdout, not stderr, got:\n%s", stderrReport) } }