Skip to content

fix(pty): capability-elevation approval prompt PTY handoff - #1254

Merged
SequeI merged 6 commits into
mainfrom
issue-1198
Jul 27, 2026
Merged

fix(pty): capability-elevation approval prompt PTY handoff#1254
SequeI merged 6 commits into
mainfrom
issue-1198

Conversation

@lukehinds

@lukehinds lukehinds commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

Pause the supervised PTY relay before Linux seccomp-notify approval prompts and restore relay mode afterward. This keeps the terminal in cooked mode while the approval backend reads from /dev/tty, avoiding garbled prompt rendering and input freezes when --capability-elevation asks for runtime access.

Also group per-notification mutable state into a small context struct so the seccomp handler stays within clippy limits.

Linked Issue

Resolves: #1198

Checklist

  • An issue exists and is linked above
  • All commits are signed-off, using DCO
  • All new code follows the project's coding standards (CLAUDE.md) and is covered by tests
  • Public-facing changes are paired with documentation updates
  • Release note has been added to CHANGELOG.md if needed

Pause the supervised PTY relay before Linux seccomp-notify approval prompts
and restore relay mode afterward. This keeps the terminal in cooked mode while
the approval backend reads from /dev/tty, avoiding garbled prompt rendering and
input freezes when --capability-elevation asks for runtime access.

Also group per-notification mutable state into a small context struct so the
seccomp handler stays within clippy limits.

Resolves: #1198

Signed-off-by: Luke Hinds <lukehinds@gmail.com>
@github-actions github-actions Bot added bug Something isn't working nono-cli size/medium labels Jun 24, 2026
@github-actions

github-actions Bot commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

PR Review Summary

Size

Metric Value
Lines added +86
Lines removed -13
Total changed 99
Classification Medium (50–300 lines)

Affected crates

  • crates/nono-cli — CLI changes. Verify argument parsing, flag documentation, and UX behaviour across supported platforms.

Blast radius — Contained

This PR touches: source code


Updated automatically on each push to this PR.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces terminal pausing and resuming around supervisor-owned approval prompts. It refactors handle_seccomp_notification to accept a new SeccompNotificationState struct, which bundles several parameters including the PTY proxy. When requesting approval, the terminal is paused and subsequently resumed using the newly added resume_terminal_after_prompt method in PtyProxy. A unit test has also been added to verify this behavior. There are no review comments, and I have no additional feedback to provide.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

@lukehinds

Copy link
Copy Markdown
Contributor Author

hold on this, as I think I resolved in the Seccomp patch. Luke to test again to clarify

@SequeI

SequeI commented Jun 29, 2026

Copy link
Copy Markdown
Member

/nogent review

@nogent-nolabs-ai nogent-nolabs-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nogent code review

No blocking issues.

Findings (not tied to a changed line):

  • 🐛 [MEDIUM · bug] crates/nono-cli/src/exec_strategy.rs:2965 — The PR fixes terminal/PTY state corruption by pausing the PTY terminal proxy during Linux seccomp-notify capability-elevation prompts. However, the generic socket-based supervisor request pathway (used on macOS and as an alternative on Linux) inside handle_supervisor_message also issues interactive approval prompts via config.approval_backend.request_approval(). Because handle_supervisor_message does not accept or pause/resume the pty proxy, any capability-elevation prompts triggered via the supervisor socket under PTY mode will still suffer from raw-mode terminal corruption, garbled rendering, and input freezes. Update handle_supervisor_message (and its callers in run_supervisor_loop) to accept pty: Option<&mut PtyProxy> and wrap its request_approval() calls with pause_terminal_for_prompt() and resume_terminal_after_prompt() protection.

Automated code + security review. CI already covers clippy, rustfmt, tests, cargo-audit and commit-lint.

Extract the pause/prompt/resume sequence into a shared helper,
request_approval_with_relay_paused, and use it from both the Linux
seccomp-notify handler and the cross-platform supervisor socket path
(handle_supervisor_message). Previously only the seccomp path paused
the relay, so approval prompts triggered over the supervisor socket
(macOS Seatbelt supervised mode) still rendered garbled and froze on
input while a terminal client was attached.

This also fixes the macOS CI failure: resume_terminal_after_prompt was
only referenced from Linux-only code, tripping -D dead-code on the
macOS binary build. Gate the UnixDatagram test import to Linux for the
same reason.

Resolves: #1198

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Luke Hinds <lukehinds@gmail.com>
@lukehinds lukehinds added this to the 0.70 milestone Jul 27, 2026

@SequeI SequeI left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, cleaner abstraction. Thanks!

@SequeI
SequeI merged commit 46db3ff into main Jul 27, 2026
28 of 29 checks passed
@SequeI
SequeI deleted the issue-1198 branch August 5, 2026 13:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working nono-cli size/medium

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Capability elevation freezes

2 participants