fix(cli): preserve terminal output during capability approval - #1527
fix(cli): preserve terminal output during capability approval#1527wswsmao wants to merge 1 commit into
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
PR Review SummarySize
Affected crates
Blast radius — ContainedThis PR touches: source code Updated automatically on each push to this PR. |
| return false; | ||
| } | ||
|
|
||
| self.drain_master_output(timeouts::pty_drain_timeout()); |
There was a problem hiding this comment.
Warning
⚡ [MEDIUM · perf] Calling self.drain_master_output(timeouts::pty_drain_timeout()) will block the parent process for up to 100 milliseconds if the PTY master is quiet/idle (which is the common case when pausing the terminal for a prompt, because the child process is blocked). This introduces a noticeable 100ms delay/stutter before displaying the interactive prompt to the user. To avoid this latency, perform a non-blocking check first (e.g. using libc::poll with a 0 timeout) to see if there is actually any pending data to relay. If there is no data, proceed immediately without waiting. Alternatively, define a much smaller quiet timeout (e.g., 5-10ms) specifically for prompt pausing rather than reusing timeouts::pty_drain_timeout().
There was a problem hiding this comment.
thanks for the contibution @wswsmao - the above from nogent is actually quite on point. As a TUI or another thread emitting output at intervals below 100 ms could keep this loop running indefinitely, preventing the approval prompt from ever appearing. SHould be pretty simple to amend though, either use a nonblocking drain or a fixed absolute deadline that incoming data cannot extend. rest of the PR is looking good 🥇
There was a problem hiding this comment.
Thanks for the review — agreed.
I replaced the post-exit quiet drain on the approval path with a
prompt-specific zero-timeout poll that relays one immediately ready PTY output
chunk. It does not wait for future output, and a continuously writing process
cannot extend the approval-prompt delay.
I added regression coverage for pending output and the single-chunk bound, then
rebased the patch on current main (0f08b477).
I also verified the interactive PTY path with
NONO_PTY_DRAIN_TIMEOUT=5000; the first approval prompt appeared in 363ms,
confirming that the prompt path no longer reuses the post-exit quiet timeout.
Environment:
- nono 0.71.0
- OpenCloudOS Stream 23 (release 2410), x86_64
- Linux 6.6.119-49.20.tl4.x86_64
- glibc 2.38
- rustc/cargo 1.95.0
- Landlock V3
Signed-off-by: abushwang <abushwang@tencent.com>
Linked Issue
Closes #1526
Summary
Fix terminal output corruption when an interactive capability approval prompt is shown while the supervised process is attached through a PTY.
Before this change, pending child-process output in the PTY could be relayed after the supervisor printed the approval prompt. Restoring terminal modes also did not normalize the cursor position or clear the parent-owned output area before displaying the prompt.
This change:
The existing capability approval behavior is unchanged. The supervisor still waits for the user to enter
yorn.No sandbox policy, Landlock behavior, seccomp-notify authorization semantics, or capability resolution logic was changed.
Agent Disclosure
This change was implemented with assistance from an AI coding agent.
The following repository guidance and files were consulted:
AGENTS.mdCLAUDE.mdCONTRIBUTING.md.github/ISSUE_TEMPLATE/bug_report.yml.github/pull_request_template.mdcrates/nono-cli/src/pty_proxy.rscrates/nono-cli/src/timeouts.rscrates/nono-cli/src/exec_strategy.rscrates/nono-cli/src/terminal_approval.rscrates/nono-cli/src/exec_strategy/supervisor_linux.rsThe implementation follows the repository requirements for error handling, terminal safety, security-sensitive behavior, formatting, testing, and DCO sign-off.
Test Plan
The following checks passed:
cargo test -p nono-clicargo clippy -p nono-cli --all-targets -- -D warningscargo fmt --all -- --checkcargo check -p nono-cligit diff --checkManual verification was also performed using a real interactive PTY with automated
y/ninput. The capability approval prompt was displayed at a stable terminal position and the process completed normally.The non-interactive
stdin=/dev/nullpath was retained as a regression comparison.Checklist