[Mikola Lysenko (mikolalysenko)]

LLM Description written by Claude Code:claude-opus-5-5

Fixes #1942

Summary

Seven PTY suites each hand-rolled the same runner: open a pty, build the hermetic CommandBuilder, spawn, answer the prompts, run a watchdog, collect. The copies had drifted. Only prompts_tty and scan_tty pinned TERM, only prompts_tty could send a second answer, and update_notifier_e2e read through its own read_to_end thread instead of PtyOutput. interactive_prompts_e2e's 20-line doc still described a reader design that no copy used any more. Register row T52; living document Part 5 (tests).

Changes (test-only)

• Added to tests/common/pty_io.rs:
  • run(args, cwd, input, timeout) -> (code, output), for the binary with one answer;
  • run_command(cmd, args, cwd, env, answers, timeout) -> PtyRun { code, signal, output }, for any hermetic::pty_command (the staged update binary, the sh SIGINT-ignored wrapper), with extra env and more than one answer.
  • Both always pin TERM=xterm-256color.
• Deleted the runners:
  • interactive_prompts_e2e: run_in_pty, run_in_pty_bytes and the stale doc;
  • prompts_tty: run_in_pty, run_in_pty_then, run_in_pty_raw, run_in_pty_inner and the Sigint enum (the single Ignored caller now builds its sh wrapper inline);
  • scan_tty: run_in_pty, run_in_pty_with;
  • remove_render, rollback_render: run_in_pty;
  • update_render: run_in_pty_env;
  • update_notifier_e2e: run_in_pty.
• Watchdog: it is now cancelled once the child is reaped, so it can no longer signal a reused pid 15-60 s later (found by /code-review; every old copy had this).
• grep -rn openpty crates/socket-patch-cli/tests now finds only common/pty_io.rs.

Red→green evidence:

• Making run_command skip every answer turns 14 of the 18 PTY tests red: 9 in the cli binary, plus the remove, rollback and update tests. They fail by watchdog at 15 s, which also shows the cancellable watchdog still fires.
• Making it skip only the second answer turns get_interactive_dialoguer_enter_accepts_first_ranked_option red.
• Both mutations were reverted.

Diff: +172 / −567 across 8 test files, with 0 production lines.

Behavior change

None in the product. In the tests, the five suites that didn't pin TERM now run under xterm-256color. All 22 PTY tests pass locally with TERM unset in the parent: 11 in cli, 4 in scan, 1 each in remove and rollback, and 2 in update.

/code-review high

• Fixed:
  • the watchdog pid reuse;
  • the doc now says the watchdog sends SIGHUP (ChildKiller::kill), names the newline and EOF that portable_pty writes when the writer drops, and spells out which answers wait for [Y/n] ;
  • the module doc now says a crate needs mod common;
  • the stale "Runner copied from…" note in update_render.
• Not changed:
  • The SIGHUP kill, the hard-coded 10 s prompt wait and the newline written on drop were in every old copy. Changing them would change test timing and behavior, which is out of scope for a consolidation.
  • run returns a tuple while run_command returns PtyRun. The tuple keeps the 20+ simple call sites one line each. env stays a parameter because the runner pins TERM before the caller's env, which lets a caller override it.

Risk

Low: test-only. It touches cfg(unix) PTY suites, so it is exercised on the Linux and macOS legs.

🤖 Generated with Claude Code

https://claude.ai/code/session_011f9aeRQMNJsvzmiHpXnUQt

---

Note

Low Risk
Test-only consolidation on cfg(unix) PTY suites; no production code paths touched.

Overview
Test-only refactor: seven Unix PTY suites each duplicated the same openpty → hermetic spawn → prompt-aware input → watchdog → collect flow. That logic now lives in tests/common/pty_io.rs as pty_io::run (built binary, one string answer) and pty_io::run_command (custom hermetic::pty_command, extra env, byte answers, optional multi-step replies, plus PtyRun with exit signal for SIGINT cases).

Call sites in interactive_prompts_e2e, prompts_tty, scan_tty, remove_render, rollback_render, update_render, and update_notifier_e2e drop local run_in_pty* helpers (~400 lines removed) and call the shared API instead. TERM=xterm-256color is always set in the runner (previously only some suites did). Sequences like menu Enter then [Y/n] confirm use answers rather than bespoke run_in_pty_then / SIGINT wrapper enums.

The timeout watchdog is cancelled after child.wait() so a late kill cannot hit a reused PID—a bug present in every old copy.

No product behavior changes; interactive PTY assertions are unchanged aside from consistent terminal typing in tests.

^{Reviewed by Cursor Bugbot for commit e8cd2b5. Configure here.}

---

Generated by Claude Code

[Mikola Lysenko (mikolalysenko)]

BugBot review

---

Generated by Claude Code

[cursor]

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

^{Reviewed by Cursor Bugbot for commit e8cd2b5. Configure here.}

[Mikola Lysenko (mikolalysenko)]

Ready for review at head e8cd2b5c9e.

• CI: all checks on the head finished with no failures (32 passed, 50 path-skipped). Mergeable, no conflicts.
• Bugbot: reviewed e8cd2b5 and found no issues. There are no unresolved review threads.
• For a reviewer: test-only refactor. The PTY suites now share one pty_io::run (it always sets TERM=xterm-256color, and the watchdog is cancelled after child.wait()). Product code is unchanged.

---

Generated by Claude Code

[Mikola Lysenko (mikolalysenko)]

Final review brief

What it does: Replaces seven hand-rolled PTY test runners with one shared pty_io::run / pty_io::run_command in tests/common/pty_io.rs. Every PTY suite now pins TERM=xterm-256color, can send more than one answer, and reads through PtyOutput. The watchdog is cancelled once the child is reaped. Before, it could send SIGHUP to a reused pid. The PR changes tests only, with no production code (+172/−567).

Risk: Low. Only tests change, in cfg(unix) PTY suites that run on the Linux and macOS legs. The one behavior change is that five suites now run under a pinned TERM.

Look here:

• common/pty_io.rs:131: run_command. The env order is: hermetic scrub, then TERM, then the caller's env.
• common/pty_io.rs:164: the watchdog is now a recv_timeout on a channel whose sender is dropped once wait() returns, so it can't fire after the child is reaped.
• common/pty_io.rs:174: sending answers. answers[0] is sent once any prompt is on screen. answers[i] is sent after the i-th [Y/n] .
• cli/prompts_tty.rs:520: the SIGINT-ignored sh wrapper. It is now built inline and replaces the Sigint enum.
• update/update_notifier_e2e.rs:619:`` this test used to read with its own read_to_end and now reads through the shared runner.

Verified:

• Read the full diff. Every removed runner matches the shared one: same PTY size, hermetic command, prompt-gated first answer, and writer drop before wait.
• The update_notifier call passes answers=[], so the writer is still closed right away, as before.
• Each test crate that uses the runner declares mod pty_io (cli, scan, remove, rollback, update).
• The only openpty left in tests/ is in common/pty_io.rs.
• CHANGELOG.md is untouched.
• CI on head: 32 passed, 50 path-skipped, 0 failed. ci-ok and clippy are green. Cursor Bugbot: success. No unresolved review threads.

Changes I made: none.

Open questions: none.

When a human approves, this PR goes straight into the merge queue: auto-merge (squash) is armed.

---

Generated by Claude Code

[Wenxin Jiang (Wenxin-Jiang)]

[agent] Reviewed e8cd2b5c9e4590465c95e5e3fceef6a85f978f7a. Checked shared PTY spawn/answer/output handling, signal tests, timeouts and migrated prompt sequences. No major correctness issues found. Validation: source and regression-test review plus the available exact-head CI checks; no local test pass is claimed.