tests: pty_run flakes when stdin isn't a terminal: pin the fd handed to script instead of inheriting it (#207) - #214
Merged
Merged
Conversation
…to `script` instead of inheriting it `script` allocates the pty for its *child*, but it first calls tcgetattr() on its own fd 0 to copy the invoking terminal's attributes onto it. What that fd is decides whether the call survives: tty tcgetattr succeeds -> fine pipe, /dev/null ENOTTY, which `script` tolerates -> fine socket ENOTSOCK/EOPNOTSUPP, which it does not -> aborts An agent harness wires its child's stdin to a socket on some invocations and to a character device on others, so the three `unregister … from a terminal` tests went red under a worker, passed on a re-run, and stayed green on both CI runners. `run pty_run …` records `script`'s own abort as the command under test failing, which is a test going red for a reason that has nothing to do with what it asserts — the habit that teaches whoever runs the suite to discount red. Reproduced deterministically by handing ./run_tests.sh a socketpair on stdin: 3 failures, verbatim the reported error. With the fix, 1131/1131 green under that same socket stdin, zero skips — the #198 tests run there, they are not skipped away. - `pty_stdin` names /dev/tty where the host has a controlling terminal, /dev/null otherwise; `pty_run` redirects from it. `script` allocates a genuine pty for the child on both arms, so `[[ -t 0 ]]` inside the command under test stays TRUE and the #198 branch is still exercised. - `require_pty` guards the three tests on a host that genuinely cannot allocate a pty: it asks `script` for one and requires the child to confirm it saw it, then skips *naming the reason*. A silent skip makes "not run" and "passed" identical in the summary, which would be worse than the flake. - `pty_run` strips the `^D` the pty echoes when `script` closes its input on the /dev/null arm; left in, it prefixes the child's first line. - New tests/pty_helper.bats pins the helper's contract, socket-stdin regression included (the probe asserts fd 0 really is a socket first, so it cannot pass vacuously). Closes #207
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
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.
Closes #207
What was actually wrong
The PM note's inference was right, and I could reproduce it — the trigger is the shape of the fd
scriptinherits on stdin, not "stdin isn't a terminal".scriptallocates the pty for its child, but first callstcgetattr()on its own fd 0 to copy the invoking terminal's attributes onto it:tcgetattr()/dev/nullENOTTYEOPNOTSUPPscript: tcgetattr/ioctl: Operation not supported on socketrun pty_run …then recordsscript's own abort asstatus=1withscript:on stdout, soassert_successfails and it reads asradio unregistermisbehaving.Reproduction
The intermittency is the agent harness handing its child a socket on some Bash invocations and a character device on others — I watched
ls -l /dev/fd/0alternate betweensrw-rw-rw-andcr--r--r--across consecutive tool calls in this very session, with the suite failing on the first and passing on the second. Forcing the socket makes it deterministic:Two things the note flagged as unverified, now checked here on macOS/BSD
script:< /dev/nullis not equivalent to a passing case by accident — it passes and the child still gets a genuine pty ([ -t 0 ]is TRUE inside).scriptonly needs a friendly fd to copy attributes from; the pty it hands the child is unaffected.< /dev/ttyis not universally available — this harness has no controlling terminal at all (/dev/tty: Device not configured), so it cannot be the only arm.The fix
Option (3) from the issue, with the preference order the evidence supports.
pty_stdinnames/dev/ttywhere the host has a controlling terminal,/dev/nullotherwise.pty_runredirects from it rather than inheriting fd 0. Both arms yield a real pty for the child, so[[ -t 0 ]]inside the command under test stays TRUE and the radio unregister: the tty gate still bypasses #187's guard — demote -t 0 from wipe-authority to stderr hint #198 branch is genuinely exercised on both.require_ptyguards the three tests for a host that truly cannot allocate a pty. It is a real probe, not a version check: it asksscriptfor a pty and requires the child to print back that it saw one on stdin. Anything short of thatskips naming the reason — a silent skip makes "not run" and "passed" identical in the summary, which would be worse than the flake. It is called from the test body, since bats'skipis inert insiderun.pty_runstrips the^Dthe pty echoes whenscriptcloses its input on the/dev/nullarm. Left in, it prefixes the child's first line (^D\b\bhello) and breaksassert_outputon output the child never produced. Found by the new tests, not by reading.Verification
Against the issue's three acceptance criteria:
1. Full suite green with a socket/pipe stdin.
< /dev/null| ./run_tests.sh(pipe)2. Still genuinely exercised, not skipped away.
skips=0in every row above — including the socket row, where the three #198 tests now run and pass rather than skip. That is the outcome the ticket asked for: the skip arm exists but nothing here reaches it. Ifrequire_ptyever fires on a CI runner, that is a bug and it will say so out loud.3. A skip states why. Pinned by a test (
require_pty skips with a stated reason rather than silently) that stubsskipand takesscriptoffPATH, asserting the reason is non-empty and names the cause.Also green:
shellcheck -x tests/helpers/*.bash tests/setup_suite.bash,tools/check-drift.sh(23 groups).New coverage —
tests/pty_helper.bats(6 tests)The socket-stdin regression asserts
[ -S /dev/fd/0 ]before callingpty_run, so it cannot pass vacuously on a host (or a future harness) that quietly hands out something friendlier. It skips with a reason ifpython3is absent; both CI runners have it. The rest pin the helper's contract: child sees a tty,pty_stdinnever passes the inherited fd through, no^Dleaks into output, exit status still propagates (theassert_failurecontract #198 relies on).Scope
Test harness only — no product code, no installed artifact. No
task-initre-run needed. No README / steering / workflow-doc changes: nothing model-facing or user-visible changed.