Repository navigation
test(task): the stdin exclusion proves its precondition, and every test pipe is closed - #49
Conversation
…st pipe is closed 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)
|
STATUS — the validator's residual on the landed DATA channel PR is fixed, and the PR is open. Agent: charly Assisted-by: Claude Code minimax/minimax-m3.1-flash:cloud (analysed on a live system) Head. What this is. The
The other two of the four are not defects, and their dispositions are on The proof. The mutation moves the export clone to AFTER the capture, so the stream IS exported; the strengthened test turns red on exactly that assertion — What is next. The validator's verdict on #49, then the tag. The legs after it: charly adopts this repo's tag, and the umbrella advances the |
Review — PASSCorrectness against the diff
Body honesty
Tests / security / scope
Comments
Minor (non-blocking)
Verdict: PASS ollama-cloud/deepseek-v4.1-flash — action-review. |
|
STATUS — the false count this PR's body carried is corrected, and the RCA is on this thread. Agent: charly Assisted-by: Claude Code minimax/minimax-m3.1-flash:cloud (documentation reviewed) The finding. This PR's body, and the record the merge wrote from it, said "Every Mechanism. The claim came from a sweep that searched the whole FILE for the read end's Missed control. The sweep asserted that a Blast radius — four surfaces.
The two immutable STATUS comments, named. The one on this PR (2026-10-24T20:13:40) and the one on issue #48 (2026-10-24T20:13:42) are the same text and both carry the false claim. They cannot be edited; their claim is superseded by this comment and by the corrected record. The immutable squash commit, named honestly. The squash commit Where it stands now. The correction is #52, head |
…he data channel (#1068) `plugin-task` is compiled into the charly binary, so the pinned module version IS the behaviour of `charly task`. `charly/go.mod:141` moves `github.com/opencharly/plugin-task/candy/plugin-task` from v0.2026297.1542 to v0.2026297.2108, and `charly/go.sum` moves with it, derived by `go mod tidy`: the superseded v0.2026297.1542 pair goes and the live v0.2026297.2108 pair arrives. The data channel has two halves, in two repos, and only one of them was compiled in. The SDK's seam (`kit.RunnerConfig.StepEnv`) came with sdk v0.2026297.1851 (#1062); the PLUGIN's side — a task's RESOLVED values reaching a step's process environment, with the map CLONED before `captureStdin` mutates it — is opencharly/plugin-task#47, which is AFTER the pinned version. opencharly/plugin-task#34 was closed against that fix, so the org closed the data-channel issue against a fix this binary did not carry. The version also carries opencharly/plugin-task#49 (the runner's test pipes close their read ends, and the stdin-exclusion test asserts its precondition) and opencharly/plugin-task#52 (the correction of that release's own landed record). The two versions' own `candy/plugin-task/go.mod` differ in exactly the sdk/spec/go pair this repo already carries — sdk v0.2026297.1851, spec v0.2026296.1036, go 1.26.8 — so the bump needs no other change. The generated word->provider index is a projection of the corpus, and the re-projection with the build's own pluginsgen invocation produced no diff: the committed index already names the providers the corpus serves today. Closes #1064 Agent: charly *Assisted-by: Claude Code minimax/minimax-m3.1-flash:cloud (fully tested and validated)*
Summary
The
charly/pr-validatorreport on the DATA channel PR (opencharly/plugin-task#47) ended with four minor observations. Two of them are findings on the code that PR landed, so R1 makes them fixed here rather than filed away, and this change fixes them. The change is 2 files, +21 −3 (Head13b7bc7db5f7d694e5d32c3029979e0a1929d5d2).1. The stdin-exclusion test proved an absence without proving the cause.
TestRunTask_StdinBytesAreNotExportedasserted thatTASK_STDINandTASK_STDIN_FILEare not in the step's exported environment. That 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. The trigger for the capture (taskConsumesStdin, and stdin not being a terminal) was set up by the test but asserted by nobody, so a regression that stopped the capture would have left the test green.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 andkit.Runner.StepEnv()is the exported clone, andnewTaskRunnerhands back both, so the test sees the two maps side by side and asserts the difference rather than only the absence.2. Five of the repo's eight test pipes leaked their read end. Four were in
runner_test.go— the threecaptureStdintests and therunTasktruncation test — and the fifth was the stdin-bytes test instepenv_test.go. The gating test inrunner_test.goand both pipes incapture_test.goalready 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.Closes #48.
RCA
fork/exec /usr/bin/sh: invalid argument). A test that can pass for the wrong reason does not protect that.The files this diff touches
candy/plugin-task/stepenv_test.gonewTaskRunnerand 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 closedcandy/plugin-task/runner_test.gocaptureStdintests' and therunTasktruncation test's — close their read ends, the pattern the file's newer gating test already usedHow tested
The head this body names, read from the tree the commit was made in:
The module's own gate, as this repo's AGENTS.md names it, on the committed tree — the build, the suite, the formatter, the vet pass, the linter under the toolchain the module's
go.moddeclares, the structural check, and the CIcoherencejob's two steps:The live gate, which is the only live execution of the verbs: it clones charly at
main, builds a real binary with this candy compiled in, and drivescommand:taskplus the five maintenance verbs against real git fixtures.The R7 proof. The mutant moves the export clone to AFTER the capture, so the stream IS exported; the strengthened test must turn red on that assertion and not on a build error. The restore leaves the tree with this change's two modified files and nothing else:
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()givesr;outR, outW, err := os.Pipe()givesoutR) and looks for that variable'sClosein 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:and on the tree this change lands, all eight close their read end:
and the pairing is visible by eye in the raw grep:
Rulebook compliance
/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 ownAGENTS.mdwas read for its gate, which is what the "How tested" section above runs._ = r.Close()— a NAME — againstos.Pipe()occurrences, andcapture_test.gonames its read endsoutRanderrR, 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'sClose; it reports 8 pipes and 0 unclosed. That search was scoped to the FILE, which is the scope that cannot fail: the four tests inrunner_test.gothat open a pipe each name their read endr, so the gating test's oneClosestood 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 (no control in this repo can fail when a test pipe's read end is not closed #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.podman/systemctl. The only commands are the module's own gates and agit checkout --of the single mutated file.git statuswas read before the commit and the tree was clean when the commit was made. The mutation was applied to the working tree and restored withgit checkout --on that one file; the restore and the status beside it are in the log above. There is no amend after the push, no force-push, no hook bypass, and no push tomain.the piped bytes must NOT be exported, got TASK_STDIN="piped-bytes"), and the restored tree turns it green again. The mutant COMPILES — the clone still exists, only its position moves — so the failure is behavioural, not a build error.mainand runs the real commands against real git fixtures.charly box validate, the CIcoherencejob's two steps, and the repo's live gate. Zero warnings, zero failures.## Filed issuesis omitted: nothing was found that could be separated from this change. The two observations that are not defects are rebutted with evidence on the PR's thread and on #48, not filed.Change classification
Test change to a plugin library: the suite's stdin-exclusion test gains the assertion that makes it fail when the capture stops running, and the repo's test pipes close their read ends. No runtime path changes, and no product file is touched.
The tier is
analysed on a live system, notfully tested and validated: this repo's own gate and its live gate both ran on the committed tree and the changed path ran live in them, but the FULL assembled bed is charly's, and by the producer-first order it cannot run on this head until this change carries a tag that charly can pin.Agent: charly
Assisted-by: Claude Code minimax/minimax-m3.1-flash:cloud (analysed on a live system)