From 13b7bc7db5f7d694e5d32c3029979e0a1929d5d2 Mon Sep 17 00:00:00 2001 From: Andreas Trawoeger Date: Sat, 24 Oct 2026 22:09:59 +0200 Subject: [PATCH] test(task): the stdin exclusion proves its precondition, and every test pipe is closed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The validator's report on the DATA channel PR left two findings on the code that PR landed, and R1 makes them fixed here rather than filed away. The first: TestRunTask_StdinBytesAreNotExported asserted an ABSENCE — that TASK_STDIN and TASK_STDIN_FILE are not in the step's exported environment — without asserting the CAUSE. The absence is satisfied by two different worlds: the capture ran and its vars were correctly excluded from the export, or the capture never ran at all and of course nothing was exported. The second is a vacuous pass, and the trigger for the capture (taskConsumesStdin, and stdin not being a terminal) was set up by the test but asserted by nobody. The test now asserts the precondition POSITIVELY: the capture ran, so the task's own merged env carries the piped bytes, while the step's exported env carries the resolved value and NOT the stream. kit.Runner.EffectiveEnv() is the task's merged env and kit.Runner.StepEnv() is the exported clone, and newTaskRunner hands back both, so the test sees the two maps side by side and asserts the difference rather than only the absence. The second: every os.Pipe() in this repo's tests leaked its read end. The four captureStdin tests did; the newer gating test already closed both ends, so this is the file's own pattern applied to its siblings — one idiom, not a new one. The proof is a mutation, and it fails on the ASSERTION: moving the export clone to AFTER the capture (so the stream IS exported) turns the test red with "the piped bytes must NOT be exported, got TASK_STDIN="piped-bytes"", and the restored tree turns it green again. The other two of the validator's four observations are not defects, and their rebuttals are on the issue: the gate arm's leftover file lives inside the gate's own $WORK tree, which the gate removes on exit, and the dependency surface the SDK bump brings is the SDK's own (loaderkit -> deploykit -> buildkitdrive), not this module's. Closes #48. Assisted-by: Claude Code minimax/minimax-m3.1-flash:cloud (analysed on a live system) --- candy/plugin-task/runner_test.go | 4 ++++ candy/plugin-task/stepenv_test.go | 20 +++++++++++++++++--- 2 files changed, 21 insertions(+), 3 deletions(-) diff --git a/candy/plugin-task/runner_test.go b/candy/plugin-task/runner_test.go index f89c6a1..ff4eb7b 100644 --- a/candy/plugin-task/runner_test.go +++ b/candy/plugin-task/runner_test.go @@ -31,6 +31,7 @@ func TestCaptureStdin_PipeExposesVars(t *testing.T) { env := map[string]string{} cap := captureStdin(env) os.Stdin = old + _ = r.Close() if v, ok := env["TASK_STDIN"]; !ok || v != "piped-input" { t.Fatalf("TASK_STDIN must be defined with the piped bytes, got %q (present=%v)", v, ok) @@ -62,6 +63,7 @@ func TestCaptureStdin_EmptyPipeStillDefinesVars(t *testing.T) { env := map[string]string{} cap := captureStdin(env) os.Stdin = old + _ = r.Close() defer cap.cleanup() if v, ok := env["TASK_STDIN"]; !ok || v != "" { @@ -92,6 +94,7 @@ func TestCaptureStdin_TruncatesAtBound(t *testing.T) { env := map[string]string{} cap := captureStdin(env) os.Stdin = old + _ = r.Close() defer cap.cleanup() if !cap.truncated { @@ -128,6 +131,7 @@ func TestRunTask_RecordsStdinTruncation(t *testing.T) { }} res, err := runTask(context.Background(), nil, ts, "t", nil, false, false) os.Stdin = old + _ = r.Close() if err != nil { t.Fatalf("runTask: %v", err) } diff --git a/candy/plugin-task/stepenv_test.go b/candy/plugin-task/stepenv_test.go index 0751889..a6b5733 100644 --- a/candy/plugin-task/stepenv_test.go +++ b/candy/plugin-task/stepenv_test.go @@ -159,17 +159,31 @@ func TestRunTask_StdinBytesAreNotExported(t *testing.T) { _, _ = w.Write([]byte("piped-bytes")) _ = w.Close() }() - defer func() { os.Stdin = old }() + defer func() { + os.Stdin = old + _ = r.Close() + }() - env, _ := captureStepEnv(t, spec.Task{ + task := spec.Task{ Description: "d", Plan: []spec.Step{{Run: "x", Op: spec.Op{ Plugin: "command", PluginInput: map[string]any{"command": `printf %s "${TASK_STDIN}"`}, }}}, - }, map[string]string{"kept": "a resolved value"}) + } + runner, stdin := newTaskRunner(nil, t.TempDir(), task, map[string]string{"kept": "a resolved value"}) + defer stdin.cleanup() + + // The PRECONDITION, asserted: the capture RAN and read the piped bytes, so the + // task's own merged env carries them. Without this the ABSENCE asserted below + // would be satisfied by a capture that never happened — a vacuous pass, and the + // reason the assertion used to prove nothing (opencharly/plugin-task#48). + if v, ok := runner.EffectiveEnv()["TASK_STDIN"]; !ok || v != "piped-bytes" { + t.Fatalf("the capture must have run and read the piped bytes, got TASK_STDIN=%q (present=%v)", v, ok) + } // The channel is ON — a resolved value is exported — while the stream is NOT. + env := runner.StepEnv() if got := env["kept"]; got != "a resolved value" { t.Fatalf("the data channel must export the resolved values, got kept=%q", got) }