fix: improve exec TTY handling and decompose into testable utilities#441
Open
AaronFeledy wants to merge 12 commits intochore/agent-dxfrom
Open
fix: improve exec TTY handling and decompose into testable utilities#441AaronFeledy wants to merge 12 commits intochore/agent-dxfrom
AaronFeledy wants to merge 12 commits intochore/agent-dxfrom
Conversation
✅ Deploy Preview for lando-core ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![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.
Autofix Details
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;
AaronFeledy
changed the title
Member
Author
Tradeoff: PTY semantics when stdout is pipedWhat this changesLando now allocates a PTY only when both The remaining edge caseIf someone pipes stdout but still drives the command from the terminal: lando ssh | tee log.txt
lando mysql | tee debug.logstdin is still attached, but the container no longer gets a PTY. That means this is not a total loss of interactivity. The narrower tradeoff is:
So tools that expect a real terminal ( Why this is still the right defaultThe previous behavior was wrong for output redirection because it could leak ANSI escapes into files and other piped output. Requiring How common is this?Probably rare. Most users either:
The ConclusionThe current version of this PR intentionally prefers clean redirected output over preserving PTY semantics when stdout is piped. That seems like the right tradeoff for the default path. If users report problems here, we can revisit with a more explicit stdin-vs-PTY policy per command. |
Member
Author
Member
Author
|
@cursor rebase this |
|
Perfect! The rebase is complete. I've successfully:
The branch now has all the latest changes from main, with your TTY allocation fix commits cleanly rebased on top. The commit history shows the branch is now based on commit   |
cursor
bot
force-pushed
the
test/tty-escape-codes-345
branch
from
4c1d21b to
64ed0cc
Compare
AaronFeledy
changed the title
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Autofix Details
Bugbot Autofix prepared fixes for both issues found in the latest run.
- ✅ Fixed: CLICOLOR_FORCE reintroduces ANSI codes in redirected output
- Removed synthetic CLICOLOR_FORCE setting because it affects all streams, not just stderr, preventing ANSI codes in redirected stdout.
- ✅ Fixed: NO_COLOR empty string not detected per spec
- Changed NO_COLOR detection from Boolean() to undefined check to properly handle empty string per no-color.org specification.
Or push these changes by commenting:
@cursor push 79dd521f05
Preview (79dd521f05)
diff --git a/test/build-exec-environment.spec.js b/test/build-exec-environment.spec.js
--- a/test/build-exec-environment.spec.js
+++ b/test/build-exec-environment.spec.js
@@ -81,16 +81,10 @@
expect(env).to.not.have.property('LINES');
});
- it('should set CLICOLOR_FORCE when stdout is piped but stderr is a terminal', () => {
+ it('should not synthetically set CLICOLOR_FORCE when stdout is piped', () => {
const ctx = {stdout: {isTTY: false, columns: 80, rows: 24}, stderr: {isTTY: true}, noColor: false};
const env = buildEnvironment(ctx);
- expect(env.CLICOLOR_FORCE).to.equal('1');
- });
-
- it('should not set CLICOLOR_FORCE when NO_COLOR is active', () => {
- const ctx = {stdout: {isTTY: false, columns: 80, rows: 24}, stderr: {isTTY: true}, noColor: true};
- const env = buildEnvironment(ctx);
- // CLICOLOR_FORCE should not be synthetically set when noColor is active
+ // CLICOLOR_FORCE should not be synthetically set because it affects all streams
expect(env).to.not.have.property('CLICOLOR_FORCE');
});
diff --git a/test/describe-context.spec.js b/test/describe-context.spec.js
--- a/test/describe-context.spec.js
+++ b/test/describe-context.spec.js
@@ -103,6 +103,14 @@
expect(describeContext().noColor).to.be.false;
});
+ it('should detect NO_COLOR when set to empty string per spec', () => {
+ process.env.NO_COLOR = '';
+ expect(describeContext().noColor).to.be.true;
+
+ delete process.env.NO_COLOR;
+ expect(describeContext().noColor).to.be.false;
+ });
+
it('should capture FORCE_COLOR from environment', () => {
process.env.FORCE_COLOR = '3';
expect(describeContext().forceColor).to.equal('3');
diff --git a/utils/build-exec-environment.js b/utils/build-exec-environment.js
--- a/utils/build-exec-environment.js
+++ b/utils/build-exec-environment.js
@@ -34,14 +34,6 @@
synthetic.LINES = String(context.stdout.rows);
}
- // If stdout is piped but stderr is still a terminal, hint to tools
- // that color on stderr is fine
- if (!context.stdout.isTTY && context.stderr.isTTY && !context.noColor) {
- if (!inherited.CLICOLOR_FORCE) {
- synthetic.CLICOLOR_FORCE = '1';
- }
- }
-
return {...inherited, ...synthetic, ...userEnv};
};
diff --git a/utils/describe-context.js b/utils/describe-context.js
--- a/utils/describe-context.js
+++ b/utils/describe-context.js
@@ -22,6 +22,6 @@
},
isNodeMode: process.lando === 'node',
ci: Boolean(process.env.CI),
- noColor: Boolean(process.env.NO_COLOR),
+ noColor: process.env.NO_COLOR !== undefined,
forceColor: process.env.FORCE_COLOR,
});You can send follow-ups to the cloud agent here.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Test helper
makeContextoverrides clobber stream defaults- Destructured stream keys from overrides before spreading to prevent clobbering merged stdin/stdout/stderr objects with their default properties.
Or push these changes by commenting:
@cursor push 24097695fe
Preview (24097695fe)
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
@@ -20,16 +20,19 @@
const {buildExecArgs} = require('../utils/build-docker-exec');
// Helper to build a minimal context object for testing
-const makeContext = (overrides = {}) => ({
- stdin: {isTTY: false, isClosed: false, ...overrides.stdin},
- stdout: {isTTY: false, columns: 80, rows: 24, ...overrides.stdout},
- stderr: {isTTY: false, ...overrides.stderr},
- isNodeMode: true,
- ci: false,
- noColor: false,
- forceColor: undefined,
- ...overrides,
-});
+const makeContext = (overrides = {}) => {
+ const {stdin, stdout, stderr, ...rest} = overrides;
+ return {
+ stdin: {isTTY: false, isClosed: false, ...stdin},
+ stdout: {isTTY: false, columns: 80, rows: 24, ...stdout},
+ stderr: {isTTY: false, ...stderr},
+ isNodeMode: true,
+ ci: false,
+ noColor: false,
+ forceColor: undefined,
+ ...rest,
+ };
+};
// Helper to build a minimal datum object for testing
const makeDatum = (overrides = {}) => ({You can send follow-ups to the cloud agent here.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Forwarding FORCE_COLOR undermines the ANSI-leak fix
- Modified buildEnvironment to skip forwarding color-forcing variables (FORCE_COLOR, NO_COLOR, CLICOLOR, CLICOLOR_FORCE) when stdout is not a TTY, preventing ANSI escape codes in redirected output.
Or push these changes by commenting:
@cursor push 98eee63899
Preview (98eee63899)
diff --git a/test/build-exec-environment.spec.js b/test/build-exec-environment.spec.js
--- a/test/build-exec-environment.spec.js
+++ b/test/build-exec-environment.spec.js
@@ -64,6 +64,27 @@
const env = buildEnvironment(ctx);
expect(env.TZ).to.equal('America/New_York');
});
+
+ it('should not forward FORCE_COLOR when stdout is not a TTY', () => {
+ process.env.FORCE_COLOR = '1';
+ const ctx = {stdout: {isTTY: false, columns: 80, rows: 24}, stderr: {isTTY: false}, noColor: false};
+ const env = buildEnvironment(ctx);
+ expect(env).to.not.have.property('FORCE_COLOR');
+ });
+
+ it('should not forward NO_COLOR when stdout is not a TTY', () => {
+ process.env.NO_COLOR = '1';
+ const ctx = {stdout: {isTTY: false, columns: 80, rows: 24}, stderr: {isTTY: false}, noColor: false};
+ const env = buildEnvironment(ctx);
+ expect(env).to.not.have.property('NO_COLOR');
+ });
+
+ it('should forward FORCE_COLOR when stdout is a TTY', () => {
+ process.env.FORCE_COLOR = '1';
+ const ctx = {stdout: {isTTY: true}, stderr: {isTTY: true}, noColor: false};
+ const env = buildEnvironment(ctx);
+ expect(env.FORCE_COLOR).to.equal('1');
+ });
});
describe('synthetic vars', () => {
@@ -88,11 +109,12 @@
expect(env).to.not.have.property('CLICOLOR_FORCE');
});
- it('should not override inherited CLICOLOR_FORCE with synthetic', () => {
+ it('should not forward CLICOLOR_FORCE when stdout is not a TTY', () => {
process.env.CLICOLOR_FORCE = '3';
const ctx = {stdout: {isTTY: false, columns: 80, rows: 24}, stderr: {isTTY: true}, noColor: false};
const env = buildEnvironment(ctx);
- expect(env.CLICOLOR_FORCE).to.equal('3');
+ // Color-forcing vars are skipped when stdout is not a TTY to prevent ANSI leakage
+ expect(env).to.not.have.property('CLICOLOR_FORCE');
});
});
diff --git a/utils/build-exec-environment.js b/utils/build-exec-environment.js
--- a/utils/build-exec-environment.js
+++ b/utils/build-exec-environment.js
@@ -23,7 +23,12 @@
module.exports = (context, userEnv = {}) => {
const inherited = {};
for (const key of forwardKeys) {
- if (process.env[key] !== undefined) inherited[key] = process.env[key];
+ if (process.env[key] !== undefined) {
+ // Skip color-forcing vars when stdout is not a TTY to prevent ANSI leakage
+ const colorVars = ['NO_COLOR', 'FORCE_COLOR', 'CLICOLOR', 'CLICOLOR_FORCE'];
+ if (!context.stdout.isTTY && colorVars.includes(key)) continue;
+ inherited[key] = process.env[key];
+ }
}
const synthetic = {};You can send follow-ups to the cloud agent here.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Autofix Details
Bugbot Autofix prepared fixes for both issues found in the latest run.
- ✅ Fixed: Context properties captured but never consumed by production code
- Removed unused properties (stderr, ci, noColor, forceColor) from describeContext and added env property to capture process.env instead.
- ✅ Fixed: Environment builder reads process.env bypassing context abstraction
- Updated buildEnvironment to read from context.env instead of process.env directly, maintaining the abstraction layer consistently.
Or push these changes by commenting:
@cursor push ae3f1bf531
Preview (ae3f1bf531)
diff --git a/test/build-exec-environment.spec.js b/test/build-exec-environment.spec.js
--- a/test/build-exec-environment.spec.js
+++ b/test/build-exec-environment.spec.js
@@ -32,56 +32,49 @@
describe('inherited vars', () => {
it('should forward TERM when set', () => {
- process.env.TERM = 'xterm-256color';
- const ctx = {stdout: {isTTY: true}, stderr: {isTTY: true}, noColor: false};
+ const ctx = {stdout: {isTTY: true}, env: {TERM: 'xterm-256color'}};
const env = buildEnvironment(ctx);
expect(env.TERM).to.equal('xterm-256color');
});
it('should not include TERM when unset', () => {
- const ctx = {stdout: {isTTY: true}, stderr: {isTTY: true}, noColor: false};
+ const ctx = {stdout: {isTTY: true}, env: {}};
const env = buildEnvironment(ctx);
expect(env).to.not.have.property('TERM');
});
it('should forward CI when set', () => {
- process.env.CI = 'true';
- const ctx = {stdout: {isTTY: false, columns: 80, rows: 24}, stderr: {isTTY: false}, noColor: false};
+ const ctx = {stdout: {isTTY: false, columns: 80, rows: 24}, env: {CI: 'true'}};
const env = buildEnvironment(ctx);
expect(env.CI).to.equal('true');
});
it('should forward DEBUG when set', () => {
- process.env.DEBUG = '*';
- const ctx = {stdout: {isTTY: true}, stderr: {isTTY: true}, noColor: false};
+ const ctx = {stdout: {isTTY: true}, env: {DEBUG: '*'}};
const env = buildEnvironment(ctx);
expect(env.DEBUG).to.equal('*');
});
it('should forward TZ when set', () => {
- process.env.TZ = 'America/New_York';
- const ctx = {stdout: {isTTY: true}, stderr: {isTTY: true}, noColor: false};
+ const ctx = {stdout: {isTTY: true}, env: {TZ: 'America/New_York'}};
const env = buildEnvironment(ctx);
expect(env.TZ).to.equal('America/New_York');
});
it('should not forward FORCE_COLOR when stdout is not a TTY', () => {
- process.env.FORCE_COLOR = '1';
- const ctx = {stdout: {isTTY: false, columns: 80, rows: 24}, stderr: {isTTY: false}, noColor: false};
+ const ctx = {stdout: {isTTY: false, columns: 80, rows: 24}, env: {FORCE_COLOR: '1'}};
const env = buildEnvironment(ctx);
expect(env).to.not.have.property('FORCE_COLOR');
});
it('should not forward CLICOLOR_FORCE when stdout is not a TTY', () => {
- process.env.CLICOLOR_FORCE = '3';
- const ctx = {stdout: {isTTY: false, columns: 80, rows: 24}, stderr: {isTTY: false}, noColor: false};
+ const ctx = {stdout: {isTTY: false, columns: 80, rows: 24}, env: {CLICOLOR_FORCE: '3'}};
const env = buildEnvironment(ctx);
expect(env).to.not.have.property('CLICOLOR_FORCE');
});
it('should still forward NO_COLOR when stdout is not a TTY', () => {
- process.env.NO_COLOR = '1';
- const ctx = {stdout: {isTTY: false, columns: 80, rows: 24}, stderr: {isTTY: false}, noColor: true};
+ const ctx = {stdout: {isTTY: false, columns: 80, rows: 24}, env: {NO_COLOR: '1'}};
const env = buildEnvironment(ctx);
expect(env.NO_COLOR).to.equal('1');
});
@@ -89,21 +82,21 @@
describe('synthetic vars', () => {
it('should set COLUMNS and LINES when stdout is not a TTY', () => {
- const ctx = {stdout: {isTTY: false, columns: 120, rows: 40}, stderr: {isTTY: false}, noColor: false};
+ const ctx = {stdout: {isTTY: false, columns: 120, rows: 40}, env: {}};
const env = buildEnvironment(ctx);
expect(env.COLUMNS).to.equal('120');
expect(env.LINES).to.equal('40');
});
it('should not set COLUMNS and LINES when stdout is a TTY', () => {
- const ctx = {stdout: {isTTY: true, columns: 120, rows: 40}, stderr: {isTTY: true}, noColor: false};
+ const ctx = {stdout: {isTTY: true, columns: 120, rows: 40}, env: {}};
const env = buildEnvironment(ctx);
expect(env).to.not.have.property('COLUMNS');
expect(env).to.not.have.property('LINES');
});
it('should not synthetically set CLICOLOR_FORCE when stdout is piped', () => {
- const ctx = {stdout: {isTTY: false, columns: 80, rows: 24}, stderr: {isTTY: true}, noColor: false};
+ const ctx = {stdout: {isTTY: false, columns: 80, rows: 24}, env: {}};
const env = buildEnvironment(ctx);
// CLICOLOR_FORCE affects all streams, not just stderr.
expect(env).to.not.have.property('CLICOLOR_FORCE');
@@ -112,20 +105,19 @@
describe('user overrides', () => {
it('should let user env override inherited vars', () => {
- process.env.TERM = 'xterm';
- const ctx = {stdout: {isTTY: true}, stderr: {isTTY: true}, noColor: false};
+ const ctx = {stdout: {isTTY: true}, env: {TERM: 'xterm'}};
const env = buildEnvironment(ctx, {TERM: 'dumb'});
expect(env.TERM).to.equal('dumb');
});
it('should let user env override synthetic vars', () => {
- const ctx = {stdout: {isTTY: false, columns: 80, rows: 24}, stderr: {isTTY: false}, noColor: false};
+ const ctx = {stdout: {isTTY: false, columns: 80, rows: 24}, env: {}};
const env = buildEnvironment(ctx, {COLUMNS: '200'});
expect(env.COLUMNS).to.equal('200');
});
it('should pass through arbitrary user vars', () => {
- const ctx = {stdout: {isTTY: true}, stderr: {isTTY: true}, noColor: false};
+ const ctx = {stdout: {isTTY: true}, env: {}};
const env = buildEnvironment(ctx, {MY_APP_VAR: 'hello'});
expect(env.MY_APP_VAR).to.equal('hello');
});
@@ -133,8 +125,7 @@
describe('precedence', () => {
it('should apply inherited < synthetic < user', () => {
- process.env.FORCE_COLOR = '1';
- const ctx = {stdout: {isTTY: false, columns: 80, rows: 24}, stderr: {isTTY: false}, noColor: false};
+ const ctx = {stdout: {isTTY: false, columns: 80, rows: 24}, env: {FORCE_COLOR: '1'}};
const env = buildEnvironment(ctx, {COLUMNS: '999', FORCE_COLOR: '3'});
// User wins over synthetic
expect(env.COLUMNS).to.equal('999');
diff --git a/test/describe-context.spec.js b/test/describe-context.spec.js
--- a/test/describe-context.spec.js
+++ b/test/describe-context.spec.js
@@ -14,33 +14,20 @@
describe('describe-context', () => {
const originalStdinIsTTY = process.stdin.isTTY;
const originalStdoutIsTTY = process.stdout.isTTY;
- const originalStderrIsTTY = process.stderr.isTTY;
const originalLando = process.lando;
- const originalEnv = {...process.env};
afterEach(() => {
process.stdin.isTTY = originalStdinIsTTY;
process.stdout.isTTY = originalStdoutIsTTY;
- process.stderr.isTTY = originalStderrIsTTY;
process.lando = originalLando;
- // Restore env vars we may have changed
- delete process.env.CI;
- delete process.env.NO_COLOR;
- delete process.env.FORCE_COLOR;
- if (originalEnv.CI !== undefined) process.env.CI = originalEnv.CI;
- if (originalEnv.NO_COLOR !== undefined) process.env.NO_COLOR = originalEnv.NO_COLOR;
- if (originalEnv.FORCE_COLOR !== undefined) process.env.FORCE_COLOR = originalEnv.FORCE_COLOR;
});
- it('should return an object with stdin, stdout, stderr, and flags', () => {
+ it('should return an object with stdin, stdout, isNodeMode, and env', () => {
const ctx = describeContext();
expect(ctx).to.have.property('stdin');
expect(ctx).to.have.property('stdout');
- expect(ctx).to.have.property('stderr');
expect(ctx).to.have.property('isNodeMode');
- expect(ctx).to.have.property('ci');
- expect(ctx).to.have.property('noColor');
- expect(ctx).to.have.property('forceColor');
+ expect(ctx).to.have.property('env');
});
it('should reflect stdin TTY state', () => {
@@ -59,14 +46,7 @@
expect(describeContext().stdout.isTTY).to.be.false;
});
- it('should reflect stderr TTY state', () => {
- process.stderr.isTTY = true;
- expect(describeContext().stderr.isTTY).to.be.true;
- process.stderr.isTTY = undefined;
- expect(describeContext().stderr.isTTY).to.be.false;
- });
-
it('should default stdout columns and rows when not available', () => {
const ctx = describeContext();
expect(ctx.stdout.columns).to.be.a('number');
@@ -86,36 +66,4 @@
delete process.lando;
expect(describeContext().isNodeMode).to.be.false;
});
-
- it('should detect CI from environment', () => {
- process.env.CI = 'true';
- expect(describeContext().ci).to.be.true;
-
- delete process.env.CI;
- expect(describeContext().ci).to.be.false;
- });
-
- it('should detect NO_COLOR from environment', () => {
- process.env.NO_COLOR = '1';
- expect(describeContext().noColor).to.be.true;
-
- delete process.env.NO_COLOR;
- expect(describeContext().noColor).to.be.false;
- });
-
- it('should detect NO_COLOR when set to an empty string', () => {
- process.env.NO_COLOR = '';
- expect(describeContext().noColor).to.be.true;
-
- delete process.env.NO_COLOR;
- expect(describeContext().noColor).to.be.false;
- });
-
- it('should capture FORCE_COLOR from environment', () => {
- process.env.FORCE_COLOR = '3';
- expect(describeContext().forceColor).to.equal('3');
-
- delete process.env.FORCE_COLOR;
- expect(describeContext().forceColor).to.be.undefined;
- });
});
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
@@ -21,16 +21,13 @@
// Helper to build a minimal context object for testing
const makeContext = (overrides = {}) => {
- const {stdin, stdout, stderr, ...rest} = overrides;
+ const {stdin, stdout, env, ...rest} = overrides;
return {
stdin: {isTTY: false, isClosed: false, ...stdin},
stdout: {isTTY: false, columns: 80, rows: 24, ...stdout},
- stderr: {isTTY: false, ...stderr},
isNodeMode: true,
- ci: false,
- noColor: false,
- forceColor: undefined,
+ env: env || {},
...rest,
};
};
diff --git a/utils/build-exec-environment.js b/utils/build-exec-environment.js
--- a/utils/build-exec-environment.js
+++ b/utils/build-exec-environment.js
@@ -25,13 +25,13 @@
module.exports = (context, userEnv = {}) => {
const inherited = {};
for (const key of forwardKeys) {
- if (process.env[key] === undefined) continue;
+ if (context.env[key] === undefined) continue;
// Redirected stdout should not inherit env vars that force color,
// or they can bypass the no-TTY safeguard and reintroduce ANSI codes.
if (!context.stdout.isTTY && forceColorKeys.includes(key)) continue;
- inherited[key] = process.env[key];
+ inherited[key] = context.env[key];
}
const synthetic = {};
diff --git a/utils/describe-context.js b/utils/describe-context.js
--- a/utils/describe-context.js
+++ b/utils/describe-context.js
@@ -17,11 +17,6 @@
columns: process.stdout.columns || 80,
rows: process.stdout.rows || 24,
},
- stderr: {
- isTTY: Boolean(process.stderr.isTTY),
- },
isNodeMode: process.lando === 'node',
- ci: Boolean(process.env.CI),
- noColor: process.env.NO_COLOR !== undefined,
- forceColor: process.env.FORCE_COLOR,
+ env: process.env,
});You can send follow-ups to the cloud agent here.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Condition reordering changes behavior for shell wrapper edge case
- Reordered conditions to check shell wrapper patterns before generic bare '&' check, ensuring ['/bin/sh', '-c', '&'] produces ['/bin/sh', '-c', ''] instead of ['/bin/sh', '-c'].
Or push these changes by commenting:
@cursor push 4ac52acd8c
Preview (4ac52acd8c)
diff --git a/utils/extract-detach.js b/utils/extract-detach.js
--- a/utils/extract-detach.js
+++ b/utils/extract-detach.js
@@ -22,17 +22,23 @@
if (parts.length === 0) return {cmd: parts, detach};
- // Trailing bare '&'
- if (parts[parts.length - 1] === '&') {
+ // Check wrapper-specific patterns first to handle edge cases correctly
+ if (parts[0] === '/etc/lando/exec.sh' && parts[parts.length - 1] === '&') {
+ // Bare '&' after exec.sh wrapper
parts.pop();
detach = true;
- // '&' appended to a shell string inside a wrapper command
} else if (parts[0] === '/etc/lando/exec.sh' && parts[parts.length - 1] && parts[parts.length - 1].endsWith('&')) {
+ // '&' appended to last argument in exec.sh wrapper
parts[parts.length - 1] = parts[parts.length - 1].slice(0, -1).trim();
detach = true;
} else if (parts[0] && parts[0].endsWith('sh') && parts[1] === '-c' && parts[2] && parts[2].endsWith('&')) {
+ // '&' appended to shell string in sh/bash -c wrapper
parts[2] = parts[2].slice(0, -1).trim();
detach = true;
+ } else if (parts[parts.length - 1] === '&') {
+ // Trailing bare '&' for all other commands
+ parts.pop();
+ detach = true;
}
return {cmd: parts, detach};You can send follow-ups to the cloud agent here.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Autofix Details
Bugbot Autofix prepared fixes for both issues found in the latest run.
- ✅ Fixed: Duplicated env logic uses different COLUMNS/LINES condition
- Replaced inline env logic in compose.js with buildEnvironment utility to ensure consistent COLUMNS/LINES injection across both compose and docker exec paths.
- ✅ Fixed: extractDetach called twice on same input
- Refactored buildExecArgs to return both args and cleaned cmd, eliminating the redundant second extractDetach call.
Or push these changes by commenting:
@cursor push 42f82794a7
Preview (42f82794a7)
diff --git a/lib/compose.js b/lib/compose.js
--- a/lib/compose.js
+++ b/lib/compose.js
@@ -4,6 +4,7 @@
const _ = require('lodash');
const describeContext = require('../utils/describe-context');
const extractDetach = require('../utils/extract-detach');
+const buildEnvironment = require('../utils/build-exec-environment');
// Helper object for flags
const composeFlags = {
@@ -143,17 +144,11 @@
opts.noTTY = !(context.stdin.isTTY && context.stdout.isTTY);
}
- // Inject terminal-size and color hints so the compose exec path
- // gets the same treatment as the docker exec path. Caller-provided
- // environment vars always win via the spread order.
- const envDefaults = {};
- if (opts.noTTY) {
- envDefaults.COLUMNS = String(context.stdout.columns);
- envDefaults.LINES = String(context.stdout.rows);
- }
- if (context.landoColorLevel === 0) {
- envDefaults.NO_COLOR = '1';
- }
+ // Use the shared buildEnvironment utility to inject terminal-size
+ // and color hints, ensuring the compose exec path gets the same
+ // treatment as the docker exec path. Caller-provided environment
+ // vars always win via the spread order.
+ const envDefaults = buildEnvironment(context);
opts.environment = {...envDefaults, ...(opts.environment || {})};
return buildShell('exec', project, compose, opts);
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
@@ -46,25 +46,25 @@
describe('docker exec (utils/build-docker-exec.js)', () => {
it('should include --tty when both stdin and stdout are TTYs', () => {
const ctx = makeContext({stdin: {isTTY: true}, stdout: {isTTY: true}});
- const args = buildExecArgs('docker', makeDatum(), ctx);
+ const {args} = buildExecArgs('docker', makeDatum(), ctx);
expect(args).to.include('--tty');
});
it('should not include --tty when stdout is not a TTY (output redirected)', () => {
const ctx = makeContext({stdin: {isTTY: true}, stdout: {isTTY: false}});
- const args = buildExecArgs('docker', makeDatum(), ctx);
+ const {args} = buildExecArgs('docker', makeDatum(), ctx);
expect(args).to.not.include('--tty');
});
it('should not include --tty when stdin is not a TTY', () => {
const ctx = makeContext({stdin: {isTTY: false}, stdout: {isTTY: true}});
- const args = buildExecArgs('docker', makeDatum(), ctx);
+ const {args} = buildExecArgs('docker', makeDatum(), ctx);
expect(args).to.not.include('--tty');
});
it('should not include --tty when neither stdin nor stdout is a TTY', () => {
const ctx = makeContext({stdin: {isTTY: false}, stdout: {isTTY: false}});
- const args = buildExecArgs('docker', makeDatum(), ctx);
+ const {args} = buildExecArgs('docker', makeDatum(), ctx);
expect(args).to.not.include('--tty');
});
});
@@ -72,26 +72,26 @@
describe('interactive mode', () => {
it('should include --interactive in node mode', () => {
const ctx = makeContext({isNodeMode: true});
- const args = buildExecArgs('docker', makeDatum(), ctx);
+ const {args} = buildExecArgs('docker', makeDatum(), ctx);
expect(args).to.include('--interactive');
});
it('should not include --interactive outside node mode', () => {
const ctx = makeContext({isNodeMode: false});
- const args = buildExecArgs('docker', makeDatum(), ctx);
+ const {args} = buildExecArgs('docker', makeDatum(), ctx);
expect(args).to.not.include('--interactive');
});
it('should not include --interactive when stdin is closed', () => {
const ctx = makeContext({isNodeMode: true, stdin: {isTTY: true, isClosed: true}});
- const args = buildExecArgs('docker', makeDatum(), ctx);
+ const {args} = buildExecArgs('docker', makeDatum(), ctx);
expect(args).to.not.include('--interactive');
});
it('should not include --interactive when detaching', () => {
const ctx = makeContext({isNodeMode: true});
const datum = makeDatum({cmd: ['sleep', '100', '&']});
- const args = buildExecArgs('docker', datum, ctx);
+ const {args} = buildExecArgs('docker', datum, ctx);
expect(args).to.include('--detach');
expect(args).to.not.include('--interactive');
});
@@ -101,7 +101,7 @@
it('should detect trailing & and add --detach', () => {
const ctx = makeContext();
const datum = makeDatum({cmd: ['sleep', '100', '&']});
- const args = buildExecArgs('docker', datum, ctx);
+ const {args} = buildExecArgs('docker', datum, ctx);
expect(args).to.include('--detach');
expect(args).to.not.include('&');
});
@@ -109,14 +109,14 @@
it('should detect appended & in shell wrappers and add --detach', () => {
const ctx = makeContext();
const datum = makeDatum({cmd: ['/bin/sh', '-c', 'sleep 100&']});
- const args = buildExecArgs('docker', datum, ctx);
+ const {args} = buildExecArgs('docker', datum, ctx);
expect(args).to.include('--detach');
});
it('should not include --tty when detaching', () => {
const ctx = makeContext({stdin: {isTTY: true}, stdout: {isTTY: true}});
const datum = makeDatum({cmd: ['sleep', '100', '&']});
- const args = buildExecArgs('docker', datum, ctx);
+ const {args} = buildExecArgs('docker', datum, ctx);
expect(args).to.include('--detach');
expect(args).to.not.include('--tty');
});
@@ -126,7 +126,7 @@
it('should include workdir when set', () => {
const ctx = makeContext();
const datum = makeDatum({opts: {user: 'root', environment: {}, workdir: '/app'}});
- const args = buildExecArgs('docker', datum, ctx);
+ const {args} = buildExecArgs('docker', datum, ctx);
const wdIdx = args.indexOf('--workdir');
expect(wdIdx).to.be.greaterThan(-1);
expect(args[wdIdx + 1]).to.equal('/app');
@@ -135,7 +135,7 @@
it('should include user', () => {
const ctx = makeContext();
const datum = makeDatum({opts: {user: 'root', environment: {}}});
- const args = buildExecArgs('docker', datum, ctx);
+ const {args} = buildExecArgs('docker', datum, ctx);
const uIdx = args.indexOf('--user');
expect(uIdx).to.be.greaterThan(-1);
expect(args[uIdx + 1]).to.equal('root');
@@ -144,7 +144,7 @@
it('should include environment variables', () => {
const ctx = makeContext();
const datum = makeDatum({opts: {user: 'root', environment: {FOO: 'bar'}}});
- const args = buildExecArgs('docker', datum, ctx);
+ const {args} = buildExecArgs('docker', datum, ctx);
expect(args).to.include('--env');
expect(args).to.include('FOO=bar');
});
@@ -152,7 +152,7 @@
it('should place container id before the command', () => {
const ctx = makeContext();
const datum = makeDatum();
- const args = buildExecArgs('docker', datum, ctx);
+ const {args} = buildExecArgs('docker', datum, ctx);
const idIdx = args.indexOf('test_container');
const cmdIdx = args.indexOf('echo');
expect(idIdx).to.be.greaterThan(-1);
@@ -161,7 +161,7 @@
it('should use the specified docker binary', () => {
const ctx = makeContext();
- const args = buildExecArgs('/usr/local/bin/docker', makeDatum(), ctx);
+ const {args} = buildExecArgs('/usr/local/bin/docker', makeDatum(), ctx);
expect(args[0]).to.equal('/usr/local/bin/docker');
expect(args[1]).to.equal('exec');
});
@@ -172,21 +172,18 @@
const ctx = makeContext();
const datum = makeDatum({cmd: ['sleep', '100', '&']});
// Simulate what the exported module function does
- buildExecArgs('docker', datum, ctx);
+ const {cmd} = buildExecArgs('docker', datum, ctx);
// The internal buildExecArgs does NOT mutate, but the exported
- // wrapper writes back. Test the extractDetach write-back that
- // the outer function performs.
- const extractDetach = require('../utils/extract-detach');
- datum.cmd = extractDetach(datum.cmd).cmd;
- expect(datum.cmd).to.eql(['sleep', '100']);
- expect(datum.cmd).to.not.include('&');
+ // wrapper writes back. Test that the returned cmd is cleaned.
+ expect(cmd).to.eql(['sleep', '100']);
+ expect(cmd).to.not.include('&');
});
it('should write cleaned cmd for shell wrapper detach', () => {
+ const ctx = makeContext();
const datum = makeDatum({cmd: ['/bin/sh', '-c', 'sleep 100&']});
- const extractDetach = require('../utils/extract-detach');
- datum.cmd = extractDetach(datum.cmd).cmd;
- expect(datum.cmd).to.eql(['/bin/sh', '-c', 'sleep 100']);
+ const {cmd} = buildExecArgs('docker', datum, ctx);
+ expect(cmd).to.eql(['/bin/sh', '-c', 'sleep 100']);
});
});
diff --git a/utils/build-docker-exec.js b/utils/build-docker-exec.js
--- a/utils/build-docker-exec.js
+++ b/utils/build-docker-exec.js
@@ -11,6 +11,9 @@
* environment propagation — reads from the context object rather than
* from process globals directly. This makes every decision testable
* with plain objects.
+ *
+ * Returns both the args array and the cleaned command for the caller
+ * to use without re-parsing.
*/
const buildExecArgs = (docker, datum, context) => {
const args = [docker, 'exec'];
@@ -46,19 +49,19 @@
args.push(datum.id);
args.push(...cmd);
- return args;
+ return {args, cmd};
};
module.exports = (injected, stdio, datum = {}) => {
const dockerBin = injected.config.dockerBin || injected._config.dockerBin;
const context = describeContext();
- const args = buildExecArgs(dockerBin, datum, context);
+ const {args, cmd} = buildExecArgs(dockerBin, datum, context);
// Write the cleaned command back to datum so callers that reuse the
// same object (e.g. build-tooling-task.js compose fallback) see it
// without the trailing '&'. This preserves the mutation contract
// the old getExecOpts() relied on.
- datum.cmd = extractDetach(datum.cmd).cmd;
+ datum.cmd = cmd;
return injected.shell.sh(args, {mode: 'attach', cstdio: stdio});
};You can send follow-ups to the cloud agent here.
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
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
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
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.
Separates four orthogonal concerns that were tangled in build-docker-exec.js and duplicated in compose.js: - describe-context.js: captures TTY/CI/color state as a plain object so downstream code never reads process globals directly - extract-detach.js: normalizes trailing '&' detection into one function, replacing 5+ branches per call site - build-exec-environment.js: layers inherited host vars, synthetic context vars, and user overrides with explicit precedence - build-docker-exec.js: now a thin assembler over those pieces, with buildExecArgs() exported for direct testing Also fixes compose.js evaluating noTTY at module load time (stale require cache bug) by computing it at call time in exports.run(). Fixes --interactive being added when detaching (nonsensical). 56 new unit tests covering all four utilities and the integrated TTY/detach/environment/command-assembly behavior.
buildEnvironment now reads from context.env instead of process.env directly, closing the abstraction gap where the context object was the single source of truth for everything except environment variables. describeContext() exposes env: process.env so production code gets the real environment while tests can pass plain objects without the save/restore dance on process.env.
…ompose path, preserve datum.cmd mutation contract
AaronFeledy
force-pushed
the
test/tty-escape-codes-345
branch
from
4b5d769 to
a4ae1b4
Compare
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: COLUMNS/LINES condition mismatches TTY allocation logic
- Changed the COLUMNS/LINES injection condition from !stdout.isTTY to !(stdin.isTTY && stdout.isTTY) to match the PTY allocation logic in buildExecArgs.
Or push these changes by commenting:
@cursor push 5a8fbb6816
Preview (5a8fbb6816)
diff --git a/test/build-exec-environment.spec.js b/test/build-exec-environment.spec.js
--- a/test/build-exec-environment.spec.js
+++ b/test/build-exec-environment.spec.js
@@ -14,8 +14,9 @@
// Helper — build a context with a controlled env so tests never
// touch process.env and don't need save/restore boilerplate.
const makeCtx = (overrides = {}) => {
- const {env, stdout, stderr, ...rest} = overrides;
+ const {env, stdin, stdout, stderr, ...rest} = overrides;
return {
+ stdin: {isTTY: true, ...stdin},
stdout: {isTTY: true, columns: 80, rows: 24, ...stdout},
stderr: {isTTY: true, ...stderr},
env: env || {},
@@ -125,9 +126,16 @@
expect(env.LINES).to.equal('40');
});
- it('should not set COLUMNS and LINES when stdout is a TTY', () => {
- const ctx = makeCtx({stdout: {isTTY: true, columns: 120, rows: 40}});
+ it('should set COLUMNS and LINES when stdin is not a TTY but stdout is', () => {
+ const ctx = makeCtx({stdin: {isTTY: false}, stdout: {isTTY: true, columns: 120, rows: 40}});
const env = buildEnvironment(ctx);
+ expect(env.COLUMNS).to.equal('120');
+ expect(env.LINES).to.equal('40');
+ });
+
+ it('should not set COLUMNS and LINES when both stdin and stdout are TTY', () => {
+ const ctx = makeCtx({stdin: {isTTY: true}, stdout: {isTTY: true, columns: 120, rows: 40}});
+ const env = buildEnvironment(ctx);
expect(env).to.not.have.property('COLUMNS');
expect(env).to.not.have.property('LINES');
});
diff --git a/utils/build-exec-environment.js b/utils/build-exec-environment.js
--- a/utils/build-exec-environment.js
+++ b/utils/build-exec-environment.js
@@ -36,7 +36,7 @@
synthetic.NO_COLOR = '1';
}
- if (!context.stdout.isTTY) {
+ if (!(context.stdin.isTTY && context.stdout.isTTY)) {
// No PTY means no SIGWINCH, but a static hint is better than nothing
synthetic.COLUMNS = String(context.stdout.columns);
synthetic.LINES = String(context.stdout.rows);You can send follow-ups to the cloud agent here.
Reviewed by Cursor Bugbot for commit 568fb81. Configure here.
| // No PTY means no SIGWINCH, but a static hint is better than nothing | ||
| synthetic.COLUMNS = String(context.stdout.columns); | ||
| synthetic.LINES = String(context.stdout.rows); | ||
| } |
There was a problem hiding this comment.
COLUMNS/LINES condition mismatches TTY allocation logic
Low Severity
build-exec-environment.js injects COLUMNS/LINES only when !context.stdout.isTTY, but the TTY allocation decision in buildExecArgs is context.stdin.isTTY && context.stdout.isTTY. When stdin is not a TTY but stdout is (e.g. piped input), no PTY is allocated yet no terminal-size hints are provided. compose.js handles this correctly by gating on opts.noTTY, which reflects both stdin and stdout state.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 568fb81. Configure here.
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
2 participants
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
Fixes ANSI escape codes leaking into redirected output (
lando composer show > file.txt) and refactors the exec handling pipeline into independently testable utilities.The bug
Lando allocated a TTY inside Docker containers based solely on
process.stdin.isTTY, ignoring whether stdout had been redirected. When a user ranlando foo > file.txtfrom a terminal, stdin was still a TTY but stdout was a file, so Docker allocated a TTY anyway and container processes emitted ANSI escape codes into the output file.The fix
Only allocate a TTY when both stdin and stdout are terminals. This matches how OpenSSH behaves and preserves colors for interactive use while producing clean output when redirected.
That alone isn't enough — a host
FORCE_COLOR=1would tell container tools to emit color regardless of the absent TTY. So redirected execs also skip inheritingFORCE_COLORandCLICOLOR_FORCE. Explicit user env config still wins.Why refactor
The one-line TTY fix had to be applied in two places (
build-docker-exec.jsandcompose.js) because both had their own copy of the detach detection and TTY logic. Worse,compose.jsevaluated its TTY default at module load time, creating a stale-cache bug on top of the original issue.This bug also can't be reproduced in CI — GitHub Actions doesn't allocate a real PTY, so
process.stdin.isTTYis alwaysfalse. The only safety net is unit tests that exercise every TTY/redirect/detach combination, and the monolithic functions couldn't be tested without mocking process globals. Decomposing into utilities that take plain data objects makes that possible.Why forward host environment
Previously,
docker execonly received the vars explicitly set indatum.opts.environment— tools inside the container had no idea about the host's terminal type, locale, timezone, or CI status. That meant container commands could mis-render output, ignoreNO_COLOR, or behave differently in CI without the user understanding why. The new environment builder forwards a curated set of host vars (TERM, TZ, LANG, CI, DEBUG, etc.) as a base layer, with user config always winning. Since the forwarding now exists, it also became the right place to gate the color-forcing vars on TTY state.What changed
utils/describe-context.jsprocessglobals.utils/extract-detach.js&detection. Wrapper commands (exec.sh,sh -c,bash -c) handle appended&as shell syntax; plain argv preserves literal&as data.utils/build-exec-environment.jscontext.env, notprocess.env.build-docker-exec.jsis now a thin assembler over those pieces, withbuildExecArgs()exported for direct testing.compose.jsfixes:docker compose execalways forces-T(no terminal to attach to)extractDetach()--interactiveno longer passed when detachingTests
63 new unit tests across 4 spec files:
test/describe-context.spec.js— 11 tests for context capturetest/extract-detach.spec.js— 12 tests for all command shapes and edge casestest/build-exec-environment.spec.js— 18 tests for env layers, redirected color handling, and precedencetest/tty-allocation.spec.js— 22 tests for TTY, interactive, detach, command assembly, and compose execTests use plain context objects with controlled
env— no process global mocking. All 258 project tests pass. New utilities have 100% coverage.Related
Note
Medium Risk
Changes core
docker exec/docker compose execargument and environment construction, which can affect interactive behavior and output formatting across many commands; mitigated by extensive new unit tests covering TTY/redirect/detach cases.Overview
Fixes redirected-command output containing ANSI escape codes by allocating a TTY only when both
stdinandstdoutare TTYs (applies to bothdocker execanddocker compose exec).Refactors exec handling into new utilities:
describe-context(snapshots TTY/CI/color/node-mode),extract-detach(centralizes trailing&→ detach detection), andbuild-exec-environment(forwards a curated set of host terminal/locale/TZ vars, addsCOLUMNS/LINESandNO_COLORwhen appropriate, and preserves caller overrides).build-docker-exec.jsandlib/compose.jsnow use these helpers, compute TTY at call time (not module load), and forcenoTTYwhen detaching.Adds comprehensive unit tests validating TTY allocation, detach parsing, context/env behavior, and compose exec flag/env injection; updates the changelog for the fix.
Reviewed by Cursor Bugbot for commit 568fb81. Bugbot is set up for automated code reviews on this repo. Configure here.