fix(api): default TERM to xterm-256color for tmux PTY attach#256
Merged
haofeif merged 1 commit intoMay 23, 2026
Merged
Conversation
Container and devcontainer environments routinely leave TERM unset or set to ``dumb``. tmux's ``attach-session`` subprocess inherited that value because the WebSocket PTY endpoint never passed ``env=`` to ``subprocess.Popen``, which broke colours, cursor positioning, and the Ink-based TUIs that Claude Code / Codex / Gemini render. The browser- side xterm.js then received text that was never written in a form it could render. Add a small ``_build_pty_env`` helper that copies the parent process environment and forces ``TERM`` to ``xterm-256color`` when the inherited value is unset, empty, or ``dumb``. Explicit non-dumb values (e.g. ``screen-256color``) are preserved verbatim. Resolves awslabs#150. Supersedes the abandoned awslabs#249, which used ``dict.setdefault`` and therefore only handled the unset case — not ``TERM=dumb`` — and shipped without unit tests (codecov flagged 0%). Tests cover the helper (unset / empty / dumb / xterm / custom value preserved / non-TERM env vars inherited) and a wiring guard that patches ``subprocess.Popen`` to assert the endpoint actually hands the corrected env to the tmux attach call.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes broken terminal rendering for the WebSocket PTY attach endpoint by ensuring the tmux attach-session subprocess receives a usable TERM value even when running in container/devcontainer environments where TERM is unset/empty/dumb.
Changes:
- Added a
_build_pty_env()helper to copy the parent environment and overrideTERMonly when it’s unset/empty/dumb. - Wired the helper into
terminal_wsby passingenv=to thesubprocess.Popencall that launchestmux attach-session. - Added unit tests covering helper behavior plus a wiring test asserting the corrected env is actually passed to
Popen.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/cli_agent_orchestrator/api/main.py |
Adds _build_pty_env() and passes env= to tmux attach to force a sane default TERM when needed. |
test/api/test_terminals.py |
Adds targeted tests for env-building behavior and a wiring test to prevent regressions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #256 +/- ##
=======================================
Coverage ? 92.23%
=======================================
Files ? 68
Lines ? 6153
Branches ? 0
=======================================
Hits ? 5675
Misses ? 478
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
haofeif
approved these changes
May 23, 2026
haofeif
left a comment
Contributor
There was a problem hiding this comment.
Thanks @call-me-ram looks great!
5 tasks
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.
Summary
The WebSocket PTY attach endpoint in
cao-servercallssubprocess.Popen(["tmux", "-u", "attach-session", ...])without anenv=argument, so the tmux child inherits the parent process environment as-is. Inside Docker and devcontainersTERMis routinely unset or set todumb, and that value flows straight through to the agent CLI's TUI renderer. The result is broken colours, garbled cursor positioning, and corrupted Ink output in the browser.This PR adds a small
_build_pty_env()helper that copies the parent environment and rewritesTERMtoxterm-256colorwhen the inherited value is unset, empty, ordumb. Explicit non-dumb values (e.g.screen-256color) are preserved.Why this isn't #249
PR #249 attempted the same fix but was closed. It used
pty_env.setdefault("TERM", "xterm-256color"), which only rewrites the unset case —TERM=dumb(the value the issue title explicitly calls out) still flowed through unchanged. It also shipped no tests, and Codecov flagged 0% patch coverage on the two changed lines.This PR:
unset, empty string, and"dumb"as equally unusable values that must be overridden. Anything else operator-set is preserved.dumb→ overridden,xterm-256color→ preserved,screen-256color→ preserved, all other env vars inherited unchanged).subprocess.Popenand asserts the endpoint actually hands the corrected env in — catches the regression of "helper exists but never gets called".Changes
src/cli_agent_orchestrator/api/main.py— new_build_pty_env()helper near the existing module-level helpers, plus the one-line wiring change at theterminal_wsPopen call to passenv=_build_pty_env().test/api/test_terminals.py— newTestBuildPtyEnv(6 tests) andTestWebSocketSubprocessTerm(1 wiring test).Test plan
uv run pytest test/api/test_terminals.py::TestBuildPtyEnv test/api/test_terminals.py::TestWebSocketSubprocessTerm -v— 7/7 pass.uv run pytest test/ --ignore=test/e2e -m "not integration"— 1713 passed, 1 skipped on Python 3.10, 3.11, and 3.12 (the CI matrix).uv run black --check src/ test/— clean.uv run isort --check-only src/ test/— clean.uv run mypy src/cli_agent_orchestrator/api/main.py— clean (no new findings on the changed module).Closes #150.