fix(learn): don't abort the review when the last blocked host isn't a child#97
Merged
Merged
Conversation
… child `sluice learn`'s wildcard-collapse offer built its subdomain list with `[ test ] && echo`, as the last stage of a pipeline inside a command substitution. When the final iteration's test failed - i.e. whenever the last blocked host was not a child of the parent being offered - the `&&` short-circuited, the while loop exited 1, pipefail carried that out of the substitution, and the plain assignment propagated it into errexit. `_learn_review` is called bare, so the whole command died there: the user saw the header, a blank line, and exit 1. No error, no prompt, no indication that anything had gone wrong. It fires whenever a collapse is offered and some other host sorts last, which is not an exotic arrangement - rows are ordered by bytes, so it depends on traffic volume rather than on anything the user controls. Uses `if` instead, so a non-matching line leaves the loop at 0. The test drives the real function sed-extracted from the built bin/sluice, with two mechanical rewrites that leave the loop untouched: the interactive read is pointed at stdin, and the "both streams non-tty" early return is forced off - without the latter, bats' piped stdout takes that return and the loop never runs. It also calls the function BARE, as production does: calling it under `|| echo` puts it in a tested context, which disarms errexit for the entire function body and makes every assertion pass against the unfixed code. Both traps were hit while writing this, hence the harness notes and the extraction guard. Verified as a real tripwire: 4 of the 5 cases fail against the unfixed slice, and the fifth - the mirror case where the last host IS a child - passes in both states, which is the control that proves the harness is not simply broken.
Pyronewbic
enabled auto-merge (rebase)
July 21, 2026 12:08
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
sluice learncan die mid-screen with no error text. The user sees the header, a blank line, and exit 1 — no prompt, no message, nothing indicating anything went wrong.The bug
The wildcard-collapse offer built its subdomain list with
[ test ] && echo, as the last stage of a pipeline inside a command substitution:subs="$(printf '%s\n' "$hosts" | while read -r h; do [ "$(parent_of "$h")" = "$p" ] && echo "$h"; done)"When the final iteration's test fails — i.e. whenever the last blocked host is not a child of the parent being offered — the
&&short-circuits, thewhileexits 1,pipefailcarries that out of the substitution, and the plain assignment propagates it into errexit._learn_reviewis called bare (src/80-learn.sh:342), so the whole command dies right there.It is not an exotic arrangement: it fires whenever a collapse is offered and some other host sorts last. Rows are ordered by bytes, so whether a user hits it depends on traffic volume rather than on anything they control or would notice.
ifinstead, so a non-matching line leaves the loop at 0.The test, and two traps hit writing it
The test drives the real function sed-extracted from the built
bin/sluice(theverify-hostbudget-unitpattern), with two mechanical rewrites that leave the loop under test byte-identical to the slice: the interactivereadis pointed at stdin, and the "both streams non-tty" early return is forced off.Both rewrites are load-bearing, and both were found by a failed first attempt:
_learn_review ... || echo "rc=$?"puts the call in a tested context, which disarms errexit for the entire function body. Every assertion again passed against the unfixed code. Production calls it bare; the test now does too.There is also a guard on the extraction itself, so a
sedthat silently matched nothing cannot make the suite pass vacuously.Verified as a real tripwire: 4 of the 5 cases fail against the unfixed slice. The 5th — the mirror case where the last host is a child — passes in both states, which is the control proving the harness is not simply broken.
419 unit tests, 0 failures.
build-checkin sync, shellcheck clean,make test-awkclean.Found while designing a demo tape: the beat's fixture happened to walk onto the landmine, and the failure mode on camera would have been a blank frame that no reviewer would read as a bug.
🤖 Generated with Claude Code
https://claude.ai/code/session_017TqRkhSka9ArLr4svJv858