Skip to content

fix(gateway): honor tty flag for interactive exec#2315

Open
emonq wants to merge 1 commit into
NVIDIA:mainfrom
emonq:2228-fix-tty-interactiveexec/emonq
Open

fix(gateway): honor tty flag for interactive exec#2315
emonq wants to merge 1 commit into
NVIDIA:mainfrom
emonq:2228-fix-tty-interactiveexec/emonq

Conversation

@emonq

@emonq emonq commented Jul 16, 2026

Copy link
Copy Markdown

Summary

Honor the requested TTY mode throughout the interactive SSH relay. When TTY is disabled, the gateway skips PTY allocation and ignores resize events, preserving pipe-based execution semantics.

Related Issue

Fixes #2228

Changes

  • Pass ExecSandboxRequest.tty through the interactive exec relay.
  • Request an SSH PTY only when tty=true.
  • Forward terminal resize events only for PTY-backed sessions.
  • Add E2E regression coverage verifying all three standard file descriptors are non-TTY with tty=false and TTY-backed with tty=true.

Testing

  • mise run pre-commit all passed.

  • cargo test -p openshell-server interactive_exec --lib — 10 passed.

  • e2e/python/test_sandbox_api.py::test_sandbox_interactive_exec_honors_tty against a Docker-backed gateway — 1 passed.

  • cargo fmt --all -- --check — passed.

  • Ruff format and lint checks — passed.

  • mise run pre-commit passes

  • Unit tests added/updated — existing focused unit tests passed; behavior is covered by E2E.

  • E2E tests added/updated (if applicable)

Checklist

  • Follows Conventional Commits
  • Commits are signed off (DCO)
  • Architecture docs updated (if applicable)

Pass the requested TTY mode through the interactive SSH relay.
Skip PTY allocation and resize forwarding when TTY is disabled,
and add regression coverage for both modes.

Signed-off-by: emonq <emonq@outlook.com>
@emonq
emonq requested review from a team, derekwaynecarr, maxamillion and mrunalp as code owners July 16, 2026 14:24
@copy-pr-bot

copy-pr-bot Bot commented Jul 16, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@github-actions

github-actions Bot commented Jul 16, 2026

Copy link
Copy Markdown

All contributors have signed the DCO ✍️ ✅
Posted by the DCO Assistant Lite bot.

@emonq

emonq commented Jul 16, 2026

Copy link
Copy Markdown
Author

I have read the DCO document and I hereby sign the DCO.

@emonq

emonq commented Jul 16, 2026

Copy link
Copy Markdown
Author

recheck

@johntmyers johntmyers self-assigned this Jul 17, 2026
@johntmyers johntmyers added the gator:in-review Gator is reviewing or awaiting PR review feedback label Jul 17, 2026
@johntmyers

Copy link
Copy Markdown
Collaborator

gator-agent

PR Review Status

Validation: This is a focused gateway bug fix for confirmed issue #2228. It honors the existing ExecSandboxRequest.tty contract without changing sandbox policy, authentication, or privilege boundaries.
Head SHA: bdfe2bc075a393cc3d56520db0836c73d97d803b

Review findings:

  • The production change correctly passes the TTY flag through the interactive relay, allocates a PTY only when requested, and ignores resize events for pipe-backed sessions.
  • Please strengthen test_sandbox_interactive_exec_honors_tty before pipeline handoff. The test currently combines stdout and stderr and never sends a streamed stdin frame, so it would pass even if stderr were returned as stdout or interactive stdin forwarding regressed. Send a stdin sentinel after the start frame, emit distinct stdout/stderr sentinels, collect the streams separately, and assert the stdin round-trip plus stream separation for tty=false while retaining the NNN/TTT checks.

Docs: No Fern update is needed; this restores behavior already documented by the protobuf contract and the existing --tty/--no-tty documentation.

Next state: gator:in-review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gator:in-review Gator is reviewing or awaiting PR review feedback

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: ExecSandboxInteractive ignores tty=false and always allocates a PTY

2 participants