Skip to content
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 27 additions & 12 deletions CHANGELOG/2026.297.2014.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Expand All @@ -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

Expand Down Expand Up @@ -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
```
Expand Down Expand Up @@ -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-<family>:<skill>` 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.
Expand Down
Loading