fix: only allocate TTY when both stdin and stdout are terminals#441
fix: only allocate TTY when both stdin and stdout are terminals#441AaronFeledy wants to merge 5 commits intomainfrom
Conversation
Adds tests to verify that `lando --help` and tooling commands do not include ANSI escape codes when stdout is redirected to a file. These tests are expected to FAIL until the fix is applied — changing TTY allocation to check `process.stdout.isTTY` instead of `process.stdin.isTTY` in compose.js and build-docker-exec.js. Ref #345
✅ Deploy Preview for lando-core ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
The leia integration tests can't reproduce the bug in CI because GitHub Actions doesn't allocate a real PTY, so stdin.isTTY is always false. Replaced with unit tests that mock process.stdin.isTTY and process.stdout.isTTY to validate both compose.js and build-docker-exec.js TTY allocation logic. Failing tests: - compose: should set noTTY=true when stdout is not a TTY - docker exec: should not include --tty when stdout is not a TTY These fail because both files check stdin.isTTY but ignore stdout.isTTY. Ref #345
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Stale require cache causes first compose test to silently mistest
- Added beforeEach hook to clear require cache before each test, ensuring fresh module evaluation with test-set TTY values.
Or push these changes by commenting:
@cursor push 65c38fe104
Preview (65c38fe104)
diff --git a/test/tty-allocation.spec.js b/test/tty-allocation.spec.js
--- a/test/tty-allocation.spec.js
+++ b/test/tty-allocation.spec.js
@@ -22,6 +22,12 @@
const originalStdinIsTTY = process.stdin.isTTY;
const originalStdoutIsTTY = process.stdout.isTTY;
+ beforeEach(() => {
+ // Clear require cache so compose.js re-evaluates with test-set TTY values
+ delete require.cache[require.resolve('./../lib/compose')];
+ delete require.cache[require.resolve('./../utils/build-docker-exec')];
+ });
+
afterEach(() => {
// Restore after each test
process.stdin.isTTY = originalStdinIsTTY;
Add beforeEach hook to clear require cache before each test runs. This ensures each test gets a fresh module evaluation with the correct TTY values, preventing the first test from using a stale cached module loaded by compose.spec.js.
Changes TTY detection in both compose exec and direct docker exec to check process.stdout.isTTY in addition to process.stdin.isTTY. Previously, running `lando foo > file.txt` from a terminal would allocate a TTY inside the container (because stdin was a TTY), causing commands like composer to emit ANSI escape codes into the output file. Now TTY is only allocated when both stdin AND stdout are terminals, matching the expected behavior: colors in interactive use, clean output when redirected. Fixes #345
AaronFeledy
changed the title
Tradeoff: TTY allocation with redirected stdoutWhat this changesPreviously, Lando allocated a TTY whenever The edge caseIf someone pipes Lando output while expecting interactive behavior: lando ssh | tee log.txt
lando mysql | tee debug.logWith this change, stdout is a pipe → no TTY allocated → no line editing, no tab completion, no arrow keys inside the container. Programs that need a TTY for input ( The current (pre-fix) behavior is wrong for output redirection ( How common is this?Probably rare. Most users either:
The Alternative consideredWe could split the check — use ConclusionThe |
Add beforeEach hook to clear require cache before each test runs. This ensures each test gets a fresh module evaluation with the correct TTY values, preventing the first test from using a stale cached module loaded by compose.spec.js. Applied via @cursor push command
2 participants
Summary
ANSI escape codes leak into redirected output because Lando allocates a TTY inside Docker containers based solely on
process.stdin.isTTY, ignoring whether stdout has been redirected.When a user runs
lando composer show > file.txtfrom a terminal, stdin is still a TTY but stdout is a file. Lando tells Docker to allocate a TTY anyway, so the container process thinks it has a full terminal and emits ANSI escape codes — which end up in the file.Root Cause
Two code paths, same bug:
lib/compose.js:29— docker-compose exec:utils/build-docker-exec.js:11— direct docker exec:Fix
Only allocate a TTY when both stdin and stdout are terminals. This preserves colors for interactive use while producing clean output when redirected to files or piped to other commands.
Why unit tests instead of Leia tests
This bug only manifests when
process.stdin.isTTYistruebutprocess.stdout.isTTYisfalse— i.e., running from a real terminal with stdout redirected to a file. GitHub Actions runners don't allocate a real PTY, soprocess.stdin.isTTYis alwaysfalsein CI. That means Leia tests can never reproduce the buggy condition — they'd pass regardless of whether the fix is applied. Unit tests let us mock the TTY state directly and validate the logic under all four stdin/stdout combinations.Tests
6 unit tests in
test/tty-allocation.spec.jscovering all TTY state combinations:All 6 pass with the fix applied. The two that validate the bug (
stdin=TTY, stdout=redirected) fail without it.Related
--ansiflag removal — fixed separately across 8 recipe plugins)Note
Low Risk
Low risk, localized change to TTY flag calculation for
docker exec/docker compose exec, with added unit tests; primary risk is minor behavioral change for edge cases where only one stream is a TTY.Overview
Prevents ANSI escape codes from leaking into redirected/piped output by changing TTY allocation to require both
process.stdin.isTTYandprocess.stdout.isTTY.This updates both the
docker compose execpath (lib/compose.jssetsnoTTYaccordingly) and the directdocker execpath (utils/build-docker-exec.jsonly adds--ttywhen both streams are TTYs), adds focused unit coverage intest/tty-allocation.spec.js, and records the fix inCHANGELOG.md.Written by Cursor Bugbot for commit 4c1d21b. This will update automatically on new commits. Configure here.