From e18e3e09cf3576916ed7e429ad5b15257b9da0c5 Mon Sep 17 00:00:00 2001 From: Andreas Trawoeger Date: Sat, 24 Oct 2026 22:46:07 +0200 Subject: [PATCH] docs: correct the test-pipe claim in the 2026.297.2014 record The record says "Every `os.Pipe()` in this repo's tests leaked its read end" and names "the four `captureStdin` tests". Measured at the scope of the function that opened each pipe, five of the repo's eight test pipes leaked their read end, not every one, and three were already closed. The four in `runner_test.go` were three `captureStdin` tests plus the `runTask` truncation test, and the fifth was in `stepenv_test.go`. The sweep pasted as the R5 evidence was scoped to the FILE, so the gating test's one `Close` stood in for the three sibling tests that also name their read end `r`: that scope reports 1 unclosed read end on the tree before the change where 5 leaked, and it cannot fail on a partial fix. The sweep now runs at the function that opened the pipe, before and after, and a control that can fail on this is filed as opencharly/plugin-task#51. The product is not touched: the merged tree already closes every read end in the function that opened it. Closes #50. Assisted-by: Claude Code minimax/minimax-m3.1-flash:cloud (documentation reviewed) --- CHANGELOG/2026.297.2014.md | 39 ++++++++++++++++++++++++++------------ 1 file changed, 27 insertions(+), 12 deletions(-) diff --git a/CHANGELOG/2026.297.2014.md b/CHANGELOG/2026.297.2014.md index 9dabc4d..33651e6 100644 --- a/CHANGELOG/2026.297.2014.md +++ b/CHANGELOG/2026.297.2014.md @@ -8,7 +8,7 @@ The `charly/pr-validator` report on the DATA channel PR (`opencharly/plugin-task 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. -**2. Every `os.Pipe()` in this repo's tests leaked its read end.** The four `captureStdin` tests did; the file's newer gating test and `capture_test.go` already closed both ends, so this change applies the file's own pattern to its siblings rather than inventing one. +**2. Five of the repo's eight test pipes leaked their read end.** Four were in `runner_test.go` — the three `captureStdin` tests and the `runTask` truncation test — and the fifth was the stdin-bytes test in `stepenv_test.go`. The gating test in `runner_test.go` and both pipes in `capture_test.go` already closed both ends, so this change applies the file's own pattern to its siblings rather than inventing one. The other two of the four observations are not defects, and their dispositions are on the PR's thread and on #48: the gate arm's leftover file lives inside the gate's own work tree (`PROJ="$WORK/proj"`), which the gate removes on exit (`trap 'rm -rf "$WORK"' EXIT`); and the dependency surface the SDK bump brings is the SDK's own (`loaderkit -> deploykit -> buildkitdrive`), not this module's. @@ -26,7 +26,7 @@ Closes #48. | file | what it does | |---|---| | `candy/plugin-task/stepenv_test.go` | the stdin test drives `newTaskRunner` and asserts both maps: the task's merged env carries the piped bytes (the capture ran), and the step's exported env does not (the stream is not exported) while the resolved value still is; the pipe's read end is closed | -| `candy/plugin-task/runner_test.go` | the four `captureStdin` tests close their pipes' read ends, the pattern the file's newer gating test already used | +| `candy/plugin-task/runner_test.go` | the four pipes in this file — the three `captureStdin` tests' and the `runTask` truncation test's — close their read ends, the pattern the file's newer gating test already used | ## How tested @@ -222,17 +222,32 @@ ok github.com/opencharly/plugin-task/candy/plugin-task 0.004s TEST_RESTORED_RC=0 ``` -The R5 sweep for this change's claim — that every `os.Pipe()` in this repo's tests closes its read end. The audit takes each pipe's read-end VARIABLE from the assignment itself (`r, w, err := os.Pipe()` gives `r`; `outR, outW, err := os.Pipe()` gives `outR`) and looks for that variable's `Close` in the same file, so a file that names its ends differently is audited rather than miscounted: +The R5 sweep for this change's claim — that every `os.Pipe()` in this repo's tests closes its read end. The audit takes each pipe's read-end VARIABLE from the assignment itself (`r, w, err := os.Pipe()` gives `r`; `outR, outW, err := os.Pipe()` gives `outR`) and looks for that variable's `Close` in the FUNCTION THAT OPENED THE PIPE, so a file that names its ends differently is audited rather than miscounted, and a close in a sibling test cannot stand in for a pipe that test did not open. On the tree BEFORE this change it names the five that leaked and the three that already closed both ends: ```console -candy/plugin-task/capture_test.go:28: os.Pipe() read end 'outR' -> closed -candy/plugin-task/capture_test.go:32: os.Pipe() read end 'errR' -> closed -candy/plugin-task/runner_test.go:22: os.Pipe() read end 'r' -> closed -candy/plugin-task/runner_test.go:57: os.Pipe() read end 'r' -> closed -candy/plugin-task/runner_test.go:85: os.Pipe() read end 'r' -> closed -candy/plugin-task/runner_test.go:117: os.Pipe() read end 'r' -> closed -candy/plugin-task/runner_test.go:169: os.Pipe() read end 'r' -> closed -candy/plugin-task/stepenv_test.go:153: os.Pipe() read end 'r' -> closed +candy/plugin-task/capture_test.go:28: captureStreams: os.Pipe() read end 'outR' -> closed +candy/plugin-task/capture_test.go:32: captureStreams: os.Pipe() read end 'errR' -> closed +candy/plugin-task/runner_test.go:22: TestCaptureStdin_PipeExposesVars: os.Pipe() read end 'r' -> NOT closed +candy/plugin-task/runner_test.go:56: TestCaptureStdin_EmptyPipeStillDefinesVars: os.Pipe() read end 'r' -> NOT closed +candy/plugin-task/runner_test.go:83: TestCaptureStdin_TruncatesAtBound: os.Pipe() read end 'r' -> NOT closed +candy/plugin-task/runner_test.go:114: TestRunTask_RecordsStdinTruncation: os.Pipe() read end 'r' -> NOT closed +candy/plugin-task/runner_test.go:165: TestTaskConsumesStdin_GatesTheRead: os.Pipe() read end 'r' -> closed +candy/plugin-task/stepenv_test.go:153: TestRunTask_StdinBytesAreNotExported: os.Pipe() read end 'r' -> NOT closed +PIPES=8 UNCLOSED=5 +PIPE_AUDIT_RC=1 +``` + +and on the tree this change lands, all eight close their read end: + +```console +candy/plugin-task/capture_test.go:28: captureStreams: os.Pipe() read end 'outR' -> closed +candy/plugin-task/capture_test.go:32: captureStreams: os.Pipe() read end 'errR' -> closed +candy/plugin-task/runner_test.go:22: TestCaptureStdin_PipeExposesVars: os.Pipe() read end 'r' -> closed +candy/plugin-task/runner_test.go:57: TestCaptureStdin_EmptyPipeStillDefinesVars: os.Pipe() read end 'r' -> closed +candy/plugin-task/runner_test.go:85: TestCaptureStdin_TruncatesAtBound: os.Pipe() read end 'r' -> closed +candy/plugin-task/runner_test.go:117: TestRunTask_RecordsStdinTruncation: os.Pipe() read end 'r' -> closed +candy/plugin-task/runner_test.go:169: TestTaskConsumesStdin_GatesTheRead: os.Pipe() read end 'r' -> closed +candy/plugin-task/stepenv_test.go:153: TestRunTask_StdinBytesAreNotExported: os.Pipe() read end 'r' -> closed PIPES=8 UNCLOSED=0 PIPE_AUDIT_RC=0 ``` @@ -273,7 +288,7 @@ stepenv_test.go:164: _ = r.Close() ## Rulebook compliance - **R0** — the skills this change's area selects were loaded before the first tool action: `/charly-internals:plugin`, `/charly-image:layer`, `/charly-internals:git-workflow`. The harness resolves no `/charly-:` registration, so the rulebook's own fallback was taken and each skill was read by path from the marketplace checkout. This repo's own `AGENTS.md` was read for its gate, which is what the "How tested" section above runs. -- **R1** — the RCA above names the mechanism, the missed control, the blast radius and the root fix, and the root fix went into the CONTROL: the mutation in the "How tested" section is the capture-path mutation the landed change never ran. Two anomalies arose in this change's own work and both were root-caused rather than worked around. The first is my own gate script: its pipe-balance check counted `_ = r.Close()` — a NAME — against `os.Pipe()` occurrences, and `capture_test.go` names its read ends `outR` and `errR`, so the check reported a false imbalance (8 pipes against 6 closes) and exited 1. The missed control was the check, not the product: a name-keyed grep is not an audit. The root fix went into the check, which now reads each pipe's read-end variable from its assignment and looks for that variable's `Close`; it reports 8 pipes and 0 unclosed. The second is the finding itself: the validator's observation on the landed PR is a finding on code this repo now ships, so it is fixed here rather than filed away. +- **R1** — the RCA above names the mechanism, the missed control, the blast radius and the root fix, and the root fix went into the CONTROL: the mutation in the "How tested" section is the capture-path mutation the landed change never ran. Two anomalies arose in this change's own work and both were root-caused rather than worked around. The first is my own gate script: its pipe-balance check counted `_ = r.Close()` — a NAME — against `os.Pipe()` occurrences, and `capture_test.go` names its read ends `outR` and `errR`, so the check reported a false imbalance (8 pipes against 6 closes) and exited 1. The missed control was the check, not the product: a name-keyed grep is not an audit. The root fix went into the check, which now reads each pipe's read-end variable from its assignment and looks for that variable's `Close`; it reports 8 pipes and 0 unclosed. That search was scoped to the FILE, which is the scope that cannot fail: the four tests in `runner_test.go` that open a pipe each name their read end `r`, so the gating test's one `Close` stood in for all four, and that scope reports 1 unclosed read end on the tree before this change where 5 leaked. The sweep above is therefore run at the FUNCTION that opened the pipe, which reports the true 5 before and 0 after, and a control that can fail on this is filed as its own issue (opencharly/plugin-task#51). The second is the finding itself: the validator's observation on the landed PR is a finding on code this repo now ships, so it is fixed here rather than filed away. - **R2** — one idiom for the pipe pattern: the close the file's newer gating test already used, applied to its siblings, not a second way to close a pipe. - **R3** — no sleep, no retry, no suppression, no tuned number, no ad-hoc `podman`/`systemctl`. The only commands are the module's own gates and a `git checkout --` of the single mutated file. - **R4** — the product is the test: the defect is in the test's assertion, and the fix is the assertion, not a note about it.