From 325578afa1164840c95789ea808129330fd41029 Mon Sep 17 00:00:00 2001 From: Aaron Feledy Date: Wed, 15 Apr 2026 17:28:59 -0500 Subject: [PATCH 01/17] support claude code --- package-lock.json | 1 + package.json | 1 + 2 files changed, 2 insertions(+) diff --git a/package-lock.json b/package-lock.json index 032b0ca7e..7dc2f07b5 100644 --- a/package-lock.json +++ b/package-lock.json @@ -7,6 +7,7 @@ "": { "name": "@lando/core", "version": "3.26.3", + "hasInstallScript": true, "license": "MIT", "dependencies": { "@lando/argv": "^1.2.0", diff --git a/package.json b/package.json index 2542acf0a..2104843d0 100644 --- a/package.json +++ b/package.json @@ -39,6 +39,7 @@ "all": true }, "scripts": { + "postinstall": "command -v claude >/dev/null 2>&1 && ln -sf AGENTS.md CLAUDE.md || true", "coverage": "nyc report --reporter=text-lcov | coveralls", "docs:build": "VPL_MVB_VERSION=$(git describe --tags --always --abbrev=1 --match=\"v[0-9].*\") vitepress build docs && npm run docs:rename-sitemap", "docs:dev": "VPL_BASE_URL=http://localhost:5173 VPL_MVB_VERSION=$(git describe --tags --always --abbrev=1 --match=\"v[0-9].*\") vitepress dev docs", From 5164eabb12f8676dc631b8795d01abca328c3949 Mon Sep 17 00:00:00 2001 From: Aaron Feledy Date: Thu, 16 Apr 2026 11:22:14 -0500 Subject: [PATCH 02/17] postinstall: replace symlink creation with cross-platform hint message --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 2104843d0..290f5314f 100644 --- a/package.json +++ b/package.json @@ -39,7 +39,7 @@ "all": true }, "scripts": { - "postinstall": "command -v claude >/dev/null 2>&1 && ln -sf AGENTS.md CLAUDE.md || true", + "postinstall": "node -e \"try{require('fs').statSync('CLAUDE.md')}catch{console.log('Tip: symlink CLAUDE.md -> AGENTS.md for Claude Code support:\\n Unix: ln -sf AGENTS.md CLAUDE.md\\n Windows: mklink CLAUDE.md AGENTS.md')}\"", "coverage": "nyc report --reporter=text-lcov | coveralls", "docs:build": "VPL_MVB_VERSION=$(git describe --tags --always --abbrev=1 --match=\"v[0-9].*\") vitepress build docs && npm run docs:rename-sitemap", "docs:dev": "VPL_BASE_URL=http://localhost:5173 VPL_MVB_VERSION=$(git describe --tags --always --abbrev=1 --match=\"v[0-9].*\") vitepress dev docs", From 576415674a6a908aaeb3bb008ae5966a03a719a8 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Thu, 16 Apr 2026 16:36:31 +0000 Subject: [PATCH 03/17] chore: remove postinstall script Co-authored-by: Aaron Feledy --- package.json | 1 - 1 file changed, 1 deletion(-) diff --git a/package.json b/package.json index 290f5314f..2542acf0a 100644 --- a/package.json +++ b/package.json @@ -39,7 +39,6 @@ "all": true }, "scripts": { - "postinstall": "node -e \"try{require('fs').statSync('CLAUDE.md')}catch{console.log('Tip: symlink CLAUDE.md -> AGENTS.md for Claude Code support:\\n Unix: ln -sf AGENTS.md CLAUDE.md\\n Windows: mklink CLAUDE.md AGENTS.md')}\"", "coverage": "nyc report --reporter=text-lcov | coveralls", "docs:build": "VPL_MVB_VERSION=$(git describe --tags --always --abbrev=1 --match=\"v[0-9].*\") vitepress build docs && npm run docs:rename-sitemap", "docs:dev": "VPL_BASE_URL=http://localhost:5173 VPL_MVB_VERSION=$(git describe --tags --always --abbrev=1 --match=\"v[0-9].*\") vitepress dev docs", From 4ba357c1a3acfcfccf724898a4be73d22cd6d7e7 Mon Sep 17 00:00:00 2001 From: Aaron Feledy Date: Fri, 27 Feb 2026 18:25:41 -0600 Subject: [PATCH 04/17] test: add ANSI escape code validation for redirected output MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- CHANGELOG.md | 1 + examples/tooling/README.md | 5 +++++ 2 files changed, 6 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 128960f40..c80925133 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ * Fixed crash when `config.yml` is empty instead of containing `{}` [#439](https://github.com/lando/core/issues/439) * Updated `lando setup` to install `docker-buildx` if missing * Fixed `buildx` built images not being available locally by adding `--load` flag +* Fixed ANSI escape codes appearing in redirected output by checking `stdout.isTTY` instead of `stdin.isTTY` for TTY allocation [#345](https://github.com/lando/core/issues/345) ## v3.26.2 - [December 17, 2025](https://github.com/lando/core/releases/tag/v3.26.2) diff --git a/examples/tooling/README.md b/examples/tooling/README.md index 83b59cc00..15049b5bf 100644 --- a/examples/tooling/README.md +++ b/examples/tooling/README.md @@ -184,6 +184,11 @@ lando cols lando lines cat cols | grep "$(tput cols)" cat lines | grep "$(tput lines)" +# Should not include ANSI escape codes in redirected lando help output +lando --help > /tmp/lando-help-output.txt 2>&1 && ! grep -P '\x1b\[' /tmp/lando-help-output.txt + +# Should not include ANSI escape codes in redirected tooling output +lando envvar > /tmp/lando-tool-output.txt 2>&1 && ! grep -P '\x1b\[' /tmp/lando-tool-output.txt ``` ## Destroy tests From 142f0b04db25e8a955147eb36a88d329531c3fe0 Mon Sep 17 00:00:00 2001 From: Aaron Feledy Date: Fri, 27 Feb 2026 18:49:06 -0600 Subject: [PATCH 05/17] test: replace leia test with unit tests for TTY allocation 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 --- examples/tooling/README.md | 5 -- test/tty-allocation.spec.js | 141 ++++++++++++++++++++++++++++++++++++ 2 files changed, 141 insertions(+), 5 deletions(-) create mode 100644 test/tty-allocation.spec.js diff --git a/examples/tooling/README.md b/examples/tooling/README.md index 15049b5bf..83b59cc00 100644 --- a/examples/tooling/README.md +++ b/examples/tooling/README.md @@ -184,11 +184,6 @@ lando cols lando lines cat cols | grep "$(tput cols)" cat lines | grep "$(tput lines)" -# Should not include ANSI escape codes in redirected lando help output -lando --help > /tmp/lando-help-output.txt 2>&1 && ! grep -P '\x1b\[' /tmp/lando-help-output.txt - -# Should not include ANSI escape codes in redirected tooling output -lando envvar > /tmp/lando-tool-output.txt 2>&1 && ! grep -P '\x1b\[' /tmp/lando-tool-output.txt ``` ## Destroy tests diff --git a/test/tty-allocation.spec.js b/test/tty-allocation.spec.js new file mode 100644 index 000000000..2c7d1f191 --- /dev/null +++ b/test/tty-allocation.spec.js @@ -0,0 +1,141 @@ +/* + * Tests for TTY allocation in docker exec and compose. + * @file tty-allocation.spec.js + * + * Validates that TTY is only allocated when BOTH stdin and stdout + * are TTYs. When stdout is redirected (e.g. `lando foo > file.txt`), + * TTY should NOT be allocated so that ANSI escape codes are not + * written to the file. + * + * @see https://github.com/lando/core/issues/345 + * @see https://github.com/lando/drupal/issues/157 + */ + +'use strict'; + +const chai = require('chai'); +const expect = chai.expect; +chai.should(); + +describe('TTY allocation', () => { + // Save originals + const originalStdinIsTTY = process.stdin.isTTY; + const originalStdoutIsTTY = process.stdout.isTTY; + + afterEach(() => { + // Restore after each test + process.stdin.isTTY = originalStdinIsTTY; + process.stdout.isTTY = originalStdoutIsTTY; + // Clear require cache so compose.js re-evaluates + delete require.cache[require.resolve('./../lib/compose')]; + delete require.cache[require.resolve('./../utils/build-docker-exec')]; + }); + + describe('compose exec (lib/compose.js)', () => { + it('should set noTTY=true when stdout is not a TTY (output redirected)', () => { + process.stdin.isTTY = true; + process.stdout.isTTY = false; + const compose = require('./../lib/compose'); + const result = compose.run( + ['docker-compose.yml'], + 'test_project', + {services: ['web'], cmd: ['echo', 'hello']}, + ); + // When noTTY is true, the -T flag should be in the command + expect(result.cmd).to.include('-T'); + }); + + it('should set noTTY=false when both stdin and stdout are TTYs', () => { + process.stdin.isTTY = true; + process.stdout.isTTY = true; + const compose = require('./../lib/compose'); + const result = compose.run( + ['docker-compose.yml'], + 'test_project', + {services: ['web'], cmd: ['echo', 'hello']}, + ); + // When both are TTY, -T should NOT be in the command + expect(result.cmd).to.not.include('-T'); + }); + + it('should set noTTY=true when stdin is not a TTY (non-interactive)', () => { + process.stdin.isTTY = false; + process.stdout.isTTY = true; + const compose = require('./../lib/compose'); + const result = compose.run( + ['docker-compose.yml'], + 'test_project', + {services: ['web'], cmd: ['echo', 'hello']}, + ); + expect(result.cmd).to.include('-T'); + }); + + it('should set noTTY=true when neither stdin nor stdout is a TTY', () => { + process.stdin.isTTY = false; + process.stdout.isTTY = false; + const compose = require('./../lib/compose'); + const result = compose.run( + ['docker-compose.yml'], + 'test_project', + {services: ['web'], cmd: ['echo', 'hello']}, + ); + expect(result.cmd).to.include('-T'); + }); + }); + + describe('docker exec (utils/build-docker-exec.js)', () => { + it('should not include --tty when stdout is not a TTY', () => { + process.stdin.isTTY = true; + process.stdout.isTTY = false; + const buildDockerExec = require('./../utils/build-docker-exec'); + + let capturedCmd; + const injected = { + config: {dockerBin: 'docker'}, + _config: {dockerBin: 'docker'}, + shell: { + sh: (cmd, opts) => { + capturedCmd = cmd; + return Promise.resolve(); + }, + }, + }; + + const datum = { + id: 'test_container', + cmd: ['echo', 'hello'], + opts: {user: 'www-data', environment: {}}, + }; + + buildDockerExec(injected, 'inherit', datum); + expect(capturedCmd).to.not.include('--tty'); + }); + + it('should include --tty when both stdin and stdout are TTYs', () => { + process.stdin.isTTY = true; + process.stdout.isTTY = true; + const buildDockerExec = require('./../utils/build-docker-exec'); + + let capturedCmd; + const injected = { + config: {dockerBin: 'docker'}, + _config: {dockerBin: 'docker'}, + shell: { + sh: (cmd, opts) => { + capturedCmd = cmd; + return Promise.resolve(); + }, + }, + }; + + const datum = { + id: 'test_container', + cmd: ['echo', 'hello'], + opts: {user: 'www-data', environment: {}}, + }; + + buildDockerExec(injected, 'inherit', datum); + expect(capturedCmd).to.include('--tty'); + }); + }); +}); From 437937df0eec1ad04d45071f98ce594e26021b1e Mon Sep 17 00:00:00 2001 From: Aaron Feledy Date: Fri, 27 Feb 2026 20:29:47 -0600 Subject: [PATCH 06/17] fix: only allocate TTY when both stdin and stdout are terminals 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 --- lib/compose.js | 2 +- utils/build-docker-exec.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/compose.js b/lib/compose.js index 34cc67c18..2a5721cfe 100644 --- a/lib/compose.js +++ b/lib/compose.js @@ -26,7 +26,7 @@ const composeFlags = { const defaultOptions = { build: {noCache: false, pull: true}, down: {removeOrphans: true, volumes: true}, - exec: {detach: false, noTTY: !process.stdin.isTTY}, + exec: {detach: false, noTTY: !(process.stdin.isTTY && process.stdout.isTTY)}, kill: {}, logs: {follow: false, timestamps: false}, ps: {q: true}, diff --git a/utils/build-docker-exec.js b/utils/build-docker-exec.js index 651f505e2..eddb6c99e 100644 --- a/utils/build-docker-exec.js +++ b/utils/build-docker-exec.js @@ -8,7 +8,7 @@ const _ = require('lodash'); const getExecOpts = (docker, datum) => { const exec = [docker, 'exec']; // Should only use this if we have to - if (process.stdin.isTTY) exec.push('--tty'); + if (process.stdin.isTTY && process.stdout.isTTY) exec.push('--tty'); // Should only set interactive in node mode if (process.lando === 'node') exec.push('--interactive'); // add workdir if we can From 66b4d83ee4bd511488cd2ff5a793458ae69bccc4 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sat, 28 Feb 2026 00:54:15 +0000 Subject: [PATCH 07/17] Fix stale require cache in TTY allocation tests 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. --- test/tty-allocation.spec.js | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/test/tty-allocation.spec.js b/test/tty-allocation.spec.js index 2c7d1f191..64edab6b2 100644 --- a/test/tty-allocation.spec.js +++ b/test/tty-allocation.spec.js @@ -22,6 +22,12 @@ describe('TTY allocation', () => { 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; From 923cfbb9fa5146386572437cbb22ba3a542b6f04 Mon Sep 17 00:00:00 2001 From: Aaron Feledy Date: Thu, 9 Apr 2026 23:11:19 -0500 Subject: [PATCH 08/17] refactor: decompose exec handling into testable utilities 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. --- lib/compose.js | 34 ++-- test/build-exec-environment.spec.js | 137 ++++++++++++++++ test/describe-context.spec.js | 113 +++++++++++++ test/extract-detach.spec.js | 87 ++++++++++ test/tty-allocation.spec.js | 245 ++++++++++++++++++++-------- utils/build-docker-exec.js | 91 ++++++----- utils/build-exec-environment.js | 49 ++++++ utils/describe-context.js | 27 +++ utils/extract-detach.js | 32 ++++ 9 files changed, 680 insertions(+), 135 deletions(-) create mode 100644 test/build-exec-environment.spec.js create mode 100644 test/describe-context.spec.js create mode 100644 test/extract-detach.spec.js create mode 100644 utils/build-exec-environment.js create mode 100644 utils/describe-context.js create mode 100644 utils/extract-detach.js diff --git a/lib/compose.js b/lib/compose.js index 2a5721cfe..da41c8a98 100644 --- a/lib/compose.js +++ b/lib/compose.js @@ -2,6 +2,8 @@ // Modules const _ = require('lodash'); +const describeContext = require('../utils/describe-context'); +const extractDetach = require('../utils/extract-detach'); // Helper object for flags const composeFlags = { @@ -26,7 +28,7 @@ const composeFlags = { const defaultOptions = { build: {noCache: false, pull: true}, down: {removeOrphans: true, volumes: true}, - exec: {detach: false, noTTY: !(process.stdin.isTTY && process.stdout.isTTY)}, + exec: {detach: false}, kill: {}, logs: {follow: false, timestamps: false}, ps: {q: true}, @@ -122,27 +124,23 @@ exports.remove = (compose, project, opts = {}) => { * Run docker compose run */ exports.run = (compose, project, opts = {}) => { - // add some deep handling for detaching - // @TODO: should we let and explicit set of opts.detach override this? - // thinking probably not because & is just not going to work the way you want without detach? - // that said we can skip this if detach is already set to true - if (opts.detach !== true) { - if (opts.cmd[0] === '/etc/lando/exec.sh' && opts.cmd[opts.cmd.length - 1] === '&') { - opts.cmd.pop(); - opts.detach = true; - } else if (opts.cmd[0].endsWith('sh') && opts.cmd[1] === '-c' && opts.cmd[2].endsWith('&')) { - opts.cmd[2] = opts.cmd[2].slice(0, -1).trim(); - opts.detach = true; - } else if (opts.cmd[0].endsWith('bash') && opts.cmd[1] === '-c' && opts.cmd[2].endsWith('&')) { - opts.cmd[2] = opts.cmd[2].slice(0, -1).trim(); - opts.detach = true; - } else if (opts.cmd[opts.cmd.length - 1] === '&') { - opts.cmd.pop(); + // Compute TTY at call time (not module load time) so that the + // check reflects the actual terminal state when the command runs. + // Callers can still override by passing noTTY explicitly. + if (opts.noTTY === undefined) { + const context = describeContext(); + opts.noTTY = !(context.stdin.isTTY && context.stdout.isTTY); + } + + // Extract detach intent from trailing '&' in the command + if (opts.detach !== true && opts.cmd) { + const result = extractDetach(opts.cmd); + if (result.detach) { + opts.cmd = result.cmd; opts.detach = true; } } - // and return return buildShell('exec', project, compose, opts); }; diff --git a/test/build-exec-environment.spec.js b/test/build-exec-environment.spec.js new file mode 100644 index 000000000..0036e404f --- /dev/null +++ b/test/build-exec-environment.spec.js @@ -0,0 +1,137 @@ +/** + * Tests for build-exec-environment.js + * @file build-exec-environment.spec.js + */ + +'use strict'; + +const chai = require('chai'); +const expect = chai.expect; +chai.should(); + +const buildEnvironment = require('../utils/build-exec-environment'); + +describe('build-exec-environment', () => { + const savedEnv = {}; + + beforeEach(() => { + // Save and clear forwarded env vars to isolate tests + for (const key of buildEnvironment.forwardKeys) { + savedEnv[key] = process.env[key]; + delete process.env[key]; + } + }); + + afterEach(() => { + // Restore env vars + for (const key of buildEnvironment.forwardKeys) { + if (savedEnv[key] !== undefined) process.env[key] = savedEnv[key]; + else delete process.env[key]; + } + }); + + 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 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 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 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 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 env = buildEnvironment(ctx); + expect(env.TZ).to.equal('America/New_York'); + }); + }); + + 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 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 env = buildEnvironment(ctx); + expect(env).to.not.have.property('COLUMNS'); + expect(env).to.not.have.property('LINES'); + }); + + it('should set CLICOLOR_FORCE when stdout is piped but stderr is a terminal', () => { + 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 + expect(env).to.not.have.property('CLICOLOR_FORCE'); + }); + + it('should not override inherited CLICOLOR_FORCE with synthetic', () => { + 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'); + }); + }); + + 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 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 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 env = buildEnvironment(ctx, {MY_APP_VAR: 'hello'}); + expect(env.MY_APP_VAR).to.equal('hello'); + }); + }); + + 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 env = buildEnvironment(ctx, {COLUMNS: '999', FORCE_COLOR: '3'}); + // User wins over synthetic + expect(env.COLUMNS).to.equal('999'); + // User wins over inherited + expect(env.FORCE_COLOR).to.equal('3'); + }); + }); +}); diff --git a/test/describe-context.spec.js b/test/describe-context.spec.js new file mode 100644 index 000000000..372094749 --- /dev/null +++ b/test/describe-context.spec.js @@ -0,0 +1,113 @@ +/** + * Tests for describe-context.js + * @file describe-context.spec.js + */ + +'use strict'; + +const chai = require('chai'); +const expect = chai.expect; +chai.should(); + +const describeContext = require('../utils/describe-context'); + +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', () => { + 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'); + }); + + it('should reflect stdin TTY state', () => { + process.stdin.isTTY = true; + expect(describeContext().stdin.isTTY).to.be.true; + + process.stdin.isTTY = undefined; + expect(describeContext().stdin.isTTY).to.be.false; + }); + + it('should reflect stdout TTY state', () => { + process.stdout.isTTY = true; + expect(describeContext().stdout.isTTY).to.be.true; + + process.stdout.isTTY = undefined; + 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'); + expect(ctx.stdout.rows).to.be.a('number'); + // Should have sensible defaults + expect(ctx.stdout.columns).to.be.at.least(1); + expect(ctx.stdout.rows).to.be.at.least(1); + }); + + it('should detect node mode from process.lando', () => { + process.lando = 'node'; + expect(describeContext().isNodeMode).to.be.true; + + process.lando = 'browser'; + expect(describeContext().isNodeMode).to.be.false; + + 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 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/extract-detach.spec.js b/test/extract-detach.spec.js new file mode 100644 index 000000000..2069d436f --- /dev/null +++ b/test/extract-detach.spec.js @@ -0,0 +1,87 @@ +/** + * Tests for extract-detach.js + * @file extract-detach.spec.js + */ + +'use strict'; + +const chai = require('chai'); +const expect = chai.expect; +chai.should(); + +const extractDetach = require('../utils/extract-detach'); + +describe('extract-detach', () => { + it('should detect bare trailing &', () => { + const result = extractDetach(['sleep', '100', '&']); + expect(result.detach).to.be.true; + expect(result.cmd).to.eql(['sleep', '100']); + }); + + it('should detect & appended to last argument', () => { + const result = extractDetach(['sleep', '100&']); + expect(result.detach).to.be.true; + expect(result.cmd).to.eql(['sleep', '100']); + }); + + it('should return detach=false when no trailing &', () => { + const result = extractDetach(['echo', 'hello']); + expect(result.detach).to.be.false; + expect(result.cmd).to.eql(['echo', 'hello']); + }); + + it('should handle empty command array', () => { + const result = extractDetach([]); + expect(result.detach).to.be.false; + expect(result.cmd).to.eql([]); + }); + + it('should work with exec.sh wrapper and bare &', () => { + const result = extractDetach(['/etc/lando/exec.sh', 'sleep', '100', '&']); + expect(result.detach).to.be.true; + expect(result.cmd).to.eql(['/etc/lando/exec.sh', 'sleep', '100']); + }); + + it('should work with exec.sh wrapper and appended &', () => { + const result = extractDetach(['/etc/lando/exec.sh', 'sleep', '100&']); + expect(result.detach).to.be.true; + expect(result.cmd).to.eql(['/etc/lando/exec.sh', 'sleep', '100']); + }); + + it('should work with sh -c wrapper', () => { + const result = extractDetach(['/bin/sh', '-c', 'sleep 100&']); + expect(result.detach).to.be.true; + expect(result.cmd).to.eql(['/bin/sh', '-c', 'sleep 100']); + }); + + it('should work with bash -c wrapper', () => { + const result = extractDetach(['/bin/bash', '-c', 'sleep 100&']); + expect(result.detach).to.be.true; + expect(result.cmd).to.eql(['/bin/bash', '-c', 'sleep 100']); + }); + + it('should not modify the original array', () => { + const original = ['sleep', '100', '&']; + const copy = [...original]; + extractDetach(original); + expect(original).to.eql(copy); + }); + + it('should ignore & in the middle of the command', () => { + const result = extractDetach(['echo', '&', 'hello']); + expect(result.detach).to.be.false; + expect(result.cmd).to.eql(['echo', '&', 'hello']); + }); + + it('should handle single-element command with &', () => { + const result = extractDetach(['&']); + expect(result.detach).to.be.true; + expect(result.cmd).to.eql([]); + }); + + it('should trim whitespace after removing &', () => { + const result = extractDetach(['sleep', '100 &']); + expect(result.detach).to.be.true; + expect(result.cmd[1]).to.equal('100'); + }); +}); diff --git a/test/tty-allocation.spec.js b/test/tty-allocation.spec.js index 64edab6b2..3fc96aa0b 100644 --- a/test/tty-allocation.spec.js +++ b/test/tty-allocation.spec.js @@ -1,5 +1,5 @@ -/* - * Tests for TTY allocation in docker exec and compose. +/** + * Tests for TTY allocation in docker exec and compose exec. * @file tty-allocation.spec.js * * Validates that TTY is only allocated when BOTH stdin and stdout @@ -17,57 +17,190 @@ const chai = require('chai'); const expect = chai.expect; chai.should(); +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, +}); + +// Helper to build a minimal datum object for testing +const makeDatum = (overrides = {}) => ({ + id: 'test_container', + cmd: ['echo', 'hello'], + opts: {user: 'www-data', environment: {}, ...overrides.opts}, + ...overrides, +}); + describe('TTY allocation', () => { - // Save originals - 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')]; + 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); + 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); + 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); + 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); + expect(args).to.not.include('--tty'); + }); + }); + + describe('interactive mode', () => { + it('should include --interactive in node mode', () => { + const ctx = makeContext({isNodeMode: true}); + 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); + 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); + 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); + expect(args).to.include('--detach'); + expect(args).to.not.include('--interactive'); + }); }); - afterEach(() => { - // Restore after each test - process.stdin.isTTY = originalStdinIsTTY; - process.stdout.isTTY = originalStdoutIsTTY; - // Clear require cache so compose.js re-evaluates - delete require.cache[require.resolve('./../lib/compose')]; - delete require.cache[require.resolve('./../utils/build-docker-exec')]; + describe('detach handling', () => { + it('should detect trailing & and add --detach', () => { + const ctx = makeContext(); + const datum = makeDatum({cmd: ['sleep', '100', '&']}); + const args = buildExecArgs('docker', datum, ctx); + expect(args).to.include('--detach'); + expect(args).to.not.include('&'); + }); + + it('should detect appended & and add --detach', () => { + const ctx = makeContext(); + const datum = makeDatum({cmd: ['sleep', '100&']}); + 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); + expect(args).to.include('--detach'); + expect(args).to.not.include('--tty'); + }); + }); + + describe('command assembly', () => { + 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 wdIdx = args.indexOf('--workdir'); + expect(wdIdx).to.be.greaterThan(-1); + expect(args[wdIdx + 1]).to.equal('/app'); + }); + + it('should include user', () => { + const ctx = makeContext(); + const datum = makeDatum({opts: {user: 'root', environment: {}}}); + const args = buildExecArgs('docker', datum, ctx); + const uIdx = args.indexOf('--user'); + expect(uIdx).to.be.greaterThan(-1); + expect(args[uIdx + 1]).to.equal('root'); + }); + + it('should include environment variables', () => { + const ctx = makeContext(); + const datum = makeDatum({opts: {user: 'root', environment: {FOO: 'bar'}}}); + const args = buildExecArgs('docker', datum, ctx); + expect(args).to.include('--env'); + expect(args).to.include('FOO=bar'); + }); + + it('should place container id before the command', () => { + const ctx = makeContext(); + const datum = makeDatum(); + const args = buildExecArgs('docker', datum, ctx); + const idIdx = args.indexOf('test_container'); + const cmdIdx = args.indexOf('echo'); + expect(idIdx).to.be.greaterThan(-1); + expect(cmdIdx).to.be.greaterThan(idIdx); + }); + + it('should use the specified docker binary', () => { + const ctx = makeContext(); + const args = buildExecArgs('/usr/local/bin/docker', makeDatum(), ctx); + expect(args[0]).to.equal('/usr/local/bin/docker'); + expect(args[1]).to.equal('exec'); + }); }); describe('compose exec (lib/compose.js)', () => { + const originalStdinIsTTY = process.stdin.isTTY; + const originalStdoutIsTTY = process.stdout.isTTY; + + afterEach(() => { + process.stdin.isTTY = originalStdinIsTTY; + process.stdout.isTTY = originalStdoutIsTTY; + }); + it('should set noTTY=true when stdout is not a TTY (output redirected)', () => { process.stdin.isTTY = true; process.stdout.isTTY = false; - const compose = require('./../lib/compose'); + const compose = require('../lib/compose'); const result = compose.run( ['docker-compose.yml'], 'test_project', {services: ['web'], cmd: ['echo', 'hello']}, ); - // When noTTY is true, the -T flag should be in the command expect(result.cmd).to.include('-T'); }); it('should set noTTY=false when both stdin and stdout are TTYs', () => { process.stdin.isTTY = true; process.stdout.isTTY = true; - const compose = require('./../lib/compose'); + const compose = require('../lib/compose'); const result = compose.run( ['docker-compose.yml'], 'test_project', {services: ['web'], cmd: ['echo', 'hello']}, ); - // When both are TTY, -T should NOT be in the command expect(result.cmd).to.not.include('-T'); }); it('should set noTTY=true when stdin is not a TTY (non-interactive)', () => { process.stdin.isTTY = false; process.stdout.isTTY = true; - const compose = require('./../lib/compose'); + const compose = require('../lib/compose'); const result = compose.run( ['docker-compose.yml'], 'test_project', @@ -79,7 +212,7 @@ describe('TTY allocation', () => { it('should set noTTY=true when neither stdin nor stdout is a TTY', () => { process.stdin.isTTY = false; process.stdout.isTTY = false; - const compose = require('./../lib/compose'); + const compose = require('../lib/compose'); const result = compose.run( ['docker-compose.yml'], 'test_project', @@ -87,61 +220,29 @@ describe('TTY allocation', () => { ); expect(result.cmd).to.include('-T'); }); - }); - describe('docker exec (utils/build-docker-exec.js)', () => { - it('should not include --tty when stdout is not a TTY', () => { + it('should allow explicit noTTY override', () => { process.stdin.isTTY = true; - process.stdout.isTTY = false; - const buildDockerExec = require('./../utils/build-docker-exec'); - - let capturedCmd; - const injected = { - config: {dockerBin: 'docker'}, - _config: {dockerBin: 'docker'}, - shell: { - sh: (cmd, opts) => { - capturedCmd = cmd; - return Promise.resolve(); - }, - }, - }; - - const datum = { - id: 'test_container', - cmd: ['echo', 'hello'], - opts: {user: 'www-data', environment: {}}, - }; - - buildDockerExec(injected, 'inherit', datum); - expect(capturedCmd).to.not.include('--tty'); + process.stdout.isTTY = true; + const compose = require('../lib/compose'); + const result = compose.run( + ['docker-compose.yml'], + 'test_project', + {services: ['web'], cmd: ['echo', 'hello'], noTTY: true}, + ); + expect(result.cmd).to.include('-T'); }); - it('should include --tty when both stdin and stdout are TTYs', () => { + it('should detect detach from trailing & in command', () => { process.stdin.isTTY = true; process.stdout.isTTY = true; - const buildDockerExec = require('./../utils/build-docker-exec'); - - let capturedCmd; - const injected = { - config: {dockerBin: 'docker'}, - _config: {dockerBin: 'docker'}, - shell: { - sh: (cmd, opts) => { - capturedCmd = cmd; - return Promise.resolve(); - }, - }, - }; - - const datum = { - id: 'test_container', - cmd: ['echo', 'hello'], - opts: {user: 'www-data', environment: {}}, - }; - - buildDockerExec(injected, 'inherit', datum); - expect(capturedCmd).to.include('--tty'); + const compose = require('../lib/compose'); + const result = compose.run( + ['docker-compose.yml'], + 'test_project', + {services: ['web'], cmd: ['sleep', '100', '&']}, + ); + expect(result.cmd).to.include('--detach'); }); }); }); diff --git a/utils/build-docker-exec.js b/utils/build-docker-exec.js index eddb6c99e..4ae233b85 100644 --- a/utils/build-docker-exec.js +++ b/utils/build-docker-exec.js @@ -1,59 +1,60 @@ 'use strict'; -const _ = require('lodash'); +const describeContext = require('./describe-context'); +const extractDetach = require('./extract-detach'); +const buildEnvironment = require('./build-exec-environment'); /* - * Build docker exec opts + * Builds the docker exec argument array. + * + * Each concern — TTY allocation, interactive mode, detach detection, + * environment propagation — reads from the context object rather than + * from process globals directly. This makes every decision testable + * with plain objects. */ -const getExecOpts = (docker, datum) => { - const exec = [docker, 'exec']; - // Should only use this if we have to - if (process.stdin.isTTY && process.stdout.isTTY) exec.push('--tty'); - // Should only set interactive in node mode - if (process.lando === 'node') exec.push('--interactive'); - // add workdir if we can +const buildExecArgs = (docker, datum, context) => { + const args = [docker, 'exec']; + const {cmd, detach} = extractDetach(datum.cmd); + + if (detach) { + args.push('--detach'); + } else { + // Allocate a PTY only when both sides are real terminals and + // we're not detaching (detach + tty is nonsensical) + if (context.stdin.isTTY && context.stdout.isTTY) { + args.push('--tty'); + } + + // Keep stdin open when running in node mode and stdin isn't closed. + // Skip when detaching — there's no stdin to attach to. + if (context.isNodeMode && !context.stdin.isClosed) { + args.push('--interactive'); + } + } + if (datum.opts.workdir) { - exec.push('--workdir'); - exec.push(datum.opts.workdir); + args.push('--workdir', datum.opts.workdir); } - // Add user - exec.push('--user'); - exec.push(datum.opts.user); - // Add envvvars - _.forEach(datum.opts.environment, (value, key) => { - exec.push('--env'); - exec.push(`${key}=${value}`); - }); - - // Assess the intention to detach for execers - if (datum.cmd[0] === '/etc/lando/exec.sh' && datum.cmd[datum.cmd.length - 1] === '&') { - datum.cmd.pop(); - exec.push('--detach'); - } else if (datum.cmd[0] === '/etc/lando/exec.sh' && datum.cmd[datum.cmd.length - 1].endsWith('&')) { - datum.cmd[datum.cmd.length - 1] = datum.cmd[datum.cmd.length - 1].slice(0, -1).trim(); - exec.push('--detach'); - // Assess the intention to detach for shell wrappers - } else if (datum.cmd[0].endsWith('sh') && datum.cmd[1] === '-c' && datum.cmd[2].endsWith('&')) { - datum.cmd[2] = datum.cmd[2].slice(0, -1).trim(); - exec.push('--detach'); - } else if (datum.cmd[0].endsWith('bash') && datum.cmd[1] === '-c' && datum.cmd[2].endsWith('&')) { - datum.cmd[2] = datum.cmd[2].slice(0, -1).trim(); - exec.push('--detach'); - // Assess the intention to detach for everything else - } else if (datum.cmd[datum.cmd.length - 1] === '&') { - datum.cmd.pop(); - exec.push('--detach'); + + args.push('--user', datum.opts.user); + + const env = buildEnvironment(context, datum.opts.environment); + for (const [key, value] of Object.entries(env)) { + args.push('--env', `${key}=${value}`); } - // Add id - exec.push(datum.id); - return exec; + args.push(datum.id); + args.push(...cmd); + + return args; }; module.exports = (injected, stdio, datum = {}) => { - // Depending on whether injected is the app or lando const dockerBin = injected.config.dockerBin || injected._config.dockerBin; - const opts = {mode: 'attach', cstdio: stdio}; - // Run run run - return injected.shell.sh(getExecOpts(dockerBin, datum).concat(datum.cmd), opts); + const context = describeContext(); + const args = buildExecArgs(dockerBin, datum, context); + return injected.shell.sh(args, {mode: 'attach', cstdio: stdio}); }; + +// Expose internals for testing +module.exports.buildExecArgs = buildExecArgs; diff --git a/utils/build-exec-environment.js b/utils/build-exec-environment.js new file mode 100644 index 000000000..cc0f3c151 --- /dev/null +++ b/utils/build-exec-environment.js @@ -0,0 +1,49 @@ +'use strict'; + +// Host variables to forward when set. These provide terminal, locale, +// and CI context to whatever runs inside the container. +const forwardKeys = [ + 'TERM', 'COLORTERM', 'TERM_PROGRAM', + 'NO_COLOR', 'FORCE_COLOR', 'CLICOLOR', 'CLICOLOR_FORCE', + 'LANG', 'LC_ALL', 'LC_CTYPE', 'LC_MESSAGES', + 'TZ', + 'CI', 'GITHUB_ACTIONS', 'GITLAB_CI', 'CIRCLECI', + 'BUILDKITE', 'JENKINS_URL', 'TRAVIS', + 'DEBUG', 'VERBOSE', +]; + +/* + * Builds the environment variables for a docker exec invocation. + * + * Three layers with explicit precedence: + * 1. inherited — host vars forwarded when set + * 2. synthetic — derived from context analysis (e.g. COLUMNS/LINES) + * 3. userEnv — explicit user overrides (always win) + */ +module.exports = (context, userEnv = {}) => { + const inherited = {}; + for (const key of forwardKeys) { + if (process.env[key] !== undefined) inherited[key] = process.env[key]; + } + + const synthetic = {}; + + if (!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); + } + + // 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}; +}; + +// Expose for testing +module.exports.forwardKeys = forwardKeys; diff --git a/utils/describe-context.js b/utils/describe-context.js new file mode 100644 index 000000000..939354e2d --- /dev/null +++ b/utils/describe-context.js @@ -0,0 +1,27 @@ +'use strict'; + +/* + * Describes the current execution context as a plain object. + * + * Every decision downstream reads from this object instead of from + * `process` directly. That single change makes the whole exec builder + * deterministic and testable with plain objects. + */ +module.exports = () => ({ + stdin: { + isTTY: Boolean(process.stdin.isTTY), + isClosed: process.stdin.readableEnded || false, + }, + stdout: { + isTTY: Boolean(process.stdout.isTTY), + 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: Boolean(process.env.NO_COLOR), + forceColor: process.env.FORCE_COLOR, +}); diff --git a/utils/extract-detach.js b/utils/extract-detach.js new file mode 100644 index 000000000..ea6cb1b13 --- /dev/null +++ b/utils/extract-detach.js @@ -0,0 +1,32 @@ +'use strict'; + +/* + * Extracts detach intent from a command array. + * + * Detects and removes a trailing '&' from the command, returning a + * clean copy and a boolean indicating whether to detach. Works for + * all command shapes: bare commands, shell wrappers (sh -c / bash -c), + * and /etc/lando/exec.sh invocations. + * + * The previous implementation had separate branches for each wrapper + * type, but in every case the '&' was either the last element or + * appended to the last element. This normalizes that in one place. + */ +module.exports = cmd => { + const parts = [...cmd]; + let detach = false; + + if (parts.length === 0) return {cmd: parts, detach}; + + // Trailing bare '&' + if (parts[parts.length - 1] === '&') { + parts.pop(); + detach = true; + // '&' appended to the last argument + } else if (parts[parts.length - 1].endsWith('&')) { + parts[parts.length - 1] = parts[parts.length - 1].slice(0, -1).trim(); + detach = true; + } + + return {cmd: parts, detach}; +}; From e28326613115d534b07012b42097b2cbcb069010 Mon Sep 17 00:00:00 2001 From: Aaron Feledy Date: Thu, 9 Apr 2026 23:40:12 -0500 Subject: [PATCH 09/17] fix detached exec tty parsing --- lib/compose.js | 17 +++++++++-------- test/extract-detach.spec.js | 14 +++++++------- test/tty-allocation.spec.js | 7 ++++--- utils/extract-detach.js | 19 +++++++++++++------ 4 files changed, 33 insertions(+), 24 deletions(-) diff --git a/lib/compose.js b/lib/compose.js index da41c8a98..186f97dbc 100644 --- a/lib/compose.js +++ b/lib/compose.js @@ -124,14 +124,6 @@ exports.remove = (compose, project, opts = {}) => { * Run docker compose run */ exports.run = (compose, project, opts = {}) => { - // Compute TTY at call time (not module load time) so that the - // check reflects the actual terminal state when the command runs. - // Callers can still override by passing noTTY explicitly. - if (opts.noTTY === undefined) { - const context = describeContext(); - opts.noTTY = !(context.stdin.isTTY && context.stdout.isTTY); - } - // Extract detach intent from trailing '&' in the command if (opts.detach !== true && opts.cmd) { const result = extractDetach(opts.cmd); @@ -141,6 +133,15 @@ exports.run = (compose, project, opts = {}) => { } } + // Detached execs should never allocate a TTY. Otherwise compute TTY + // at call time so the decision reflects the current terminal state. + if (opts.detach === true) { + opts.noTTY = true; + } else if (opts.noTTY === undefined) { + const context = describeContext(); + opts.noTTY = !(context.stdin.isTTY && context.stdout.isTTY); + } + return buildShell('exec', project, compose, opts); }; diff --git a/test/extract-detach.spec.js b/test/extract-detach.spec.js index 2069d436f..9fa96d737 100644 --- a/test/extract-detach.spec.js +++ b/test/extract-detach.spec.js @@ -18,10 +18,10 @@ describe('extract-detach', () => { expect(result.cmd).to.eql(['sleep', '100']); }); - it('should detect & appended to last argument', () => { - const result = extractDetach(['sleep', '100&']); - expect(result.detach).to.be.true; - expect(result.cmd).to.eql(['sleep', '100']); + it('should not treat literal trailing & in a plain argv argument as detach', () => { + const result = extractDetach(['curl', 'https://example.test/?a=1&']); + expect(result.detach).to.be.false; + expect(result.cmd).to.eql(['curl', 'https://example.test/?a=1&']); }); it('should return detach=false when no trailing &', () => { @@ -79,9 +79,9 @@ describe('extract-detach', () => { expect(result.cmd).to.eql([]); }); - it('should trim whitespace after removing &', () => { - const result = extractDetach(['sleep', '100 &']); + it('should trim whitespace after removing & from shell wrappers', () => { + const result = extractDetach(['/bin/sh', '-c', 'sleep 100 &']); expect(result.detach).to.be.true; - expect(result.cmd[1]).to.equal('100'); + expect(result.cmd[2]).to.equal('sleep 100'); }); }); diff --git a/test/tty-allocation.spec.js b/test/tty-allocation.spec.js index 3fc96aa0b..784cce5fa 100644 --- a/test/tty-allocation.spec.js +++ b/test/tty-allocation.spec.js @@ -103,9 +103,9 @@ describe('TTY allocation', () => { expect(args).to.not.include('&'); }); - it('should detect appended & and add --detach', () => { + it('should detect appended & in shell wrappers and add --detach', () => { const ctx = makeContext(); - const datum = makeDatum({cmd: ['sleep', '100&']}); + const datum = makeDatum({cmd: ['/bin/sh', '-c', 'sleep 100&']}); const args = buildExecArgs('docker', datum, ctx); expect(args).to.include('--detach'); }); @@ -233,7 +233,7 @@ describe('TTY allocation', () => { expect(result.cmd).to.include('-T'); }); - it('should detect detach from trailing & in command', () => { + it('should detect detach from trailing & in command and force noTTY', () => { process.stdin.isTTY = true; process.stdout.isTTY = true; const compose = require('../lib/compose'); @@ -243,6 +243,7 @@ describe('TTY allocation', () => { {services: ['web'], cmd: ['sleep', '100', '&']}, ); expect(result.cmd).to.include('--detach'); + expect(result.cmd).to.include('-T'); }); }); }); diff --git a/utils/extract-detach.js b/utils/extract-detach.js index ea6cb1b13..6e4734d12 100644 --- a/utils/extract-detach.js +++ b/utils/extract-detach.js @@ -3,10 +3,14 @@ /* * Extracts detach intent from a command array. * - * Detects and removes a trailing '&' from the command, returning a - * clean copy and a boolean indicating whether to detach. Works for - * all command shapes: bare commands, shell wrappers (sh -c / bash -c), - * and /etc/lando/exec.sh invocations. + * Detects and removes detach syntax from the command, returning a + * clean copy and a boolean indicating whether to detach. + * + * A bare trailing '&' is only meaningful as shell syntax, so we accept + * it for all command shapes. An appended '&' is only shell syntax for + * wrapper-style commands where the final command is itself a shell + * string (`sh -c`, `bash -c`, or `/etc/lando/exec.sh`). For plain argv + * commands, a trailing '&' can be literal data and must be preserved. * * The previous implementation had separate branches for each wrapper * type, but in every case the '&' was either the last element or @@ -22,10 +26,13 @@ module.exports = cmd => { if (parts[parts.length - 1] === '&') { parts.pop(); detach = true; - // '&' appended to the last argument - } else if (parts[parts.length - 1].endsWith('&')) { + // '&' 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('&')) { 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('&')) { + parts[2] = parts[2].slice(0, -1).trim(); + detach = true; } return {cmd: parts, detach}; From 75c839a15c0ed56255eb59f6c8ff127cd4874a55 Mon Sep 17 00:00:00 2001 From: Aaron Feledy Date: Fri, 10 Apr 2026 00:04:25 -0500 Subject: [PATCH 10/17] fix exec color env detection --- test/build-exec-environment.spec.js | 10 ++-------- test/describe-context.spec.js | 8 ++++++++ utils/build-exec-environment.js | 8 -------- utils/describe-context.js | 2 +- 4 files changed, 11 insertions(+), 17 deletions(-) diff --git a/test/build-exec-environment.spec.js b/test/build-exec-environment.spec.js index 0036e404f..4101b745b 100644 --- a/test/build-exec-environment.spec.js +++ b/test/build-exec-environment.spec.js @@ -81,16 +81,10 @@ describe('build-exec-environment', () => { 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 affects all streams, not just stderr. expect(env).to.not.have.property('CLICOLOR_FORCE'); }); diff --git a/test/describe-context.spec.js b/test/describe-context.spec.js index 372094749..0ea83146b 100644 --- a/test/describe-context.spec.js +++ b/test/describe-context.spec.js @@ -103,6 +103,14 @@ describe('describe-context', () => { 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'); diff --git a/utils/build-exec-environment.js b/utils/build-exec-environment.js index cc0f3c151..c95034e1b 100644 --- a/utils/build-exec-environment.js +++ b/utils/build-exec-environment.js @@ -34,14 +34,6 @@ module.exports = (context, userEnv = {}) => { 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 index 939354e2d..e742091f4 100644 --- a/utils/describe-context.js +++ b/utils/describe-context.js @@ -22,6 +22,6 @@ module.exports = () => ({ }, 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, }); From 5d8f62d611ec5bcd7a77d99123f2400bed408b40 Mon Sep 17 00:00:00 2001 From: Aaron Feledy Date: Fri, 10 Apr 2026 00:21:07 -0500 Subject: [PATCH 11/17] fix tty test context helper --- test/tty-allocation.spec.js | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/test/tty-allocation.spec.js b/test/tty-allocation.spec.js index 784cce5fa..5c751442a 100644 --- a/test/tty-allocation.spec.js +++ b/test/tty-allocation.spec.js @@ -20,16 +20,20 @@ chai.should(); 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 = {}) => ({ From a800a888490088ff99b8c381b84ce97c9413ac51 Mon Sep 17 00:00:00 2001 From: Aaron Feledy Date: Fri, 10 Apr 2026 01:30:54 -0500 Subject: [PATCH 12/17] fix redirected exec color env forwarding --- test/build-exec-environment.spec.js | 28 +++++++++++++++++++++------- utils/build-exec-environment.js | 10 +++++++++- 2 files changed, 30 insertions(+), 8 deletions(-) diff --git a/test/build-exec-environment.spec.js b/test/build-exec-environment.spec.js index 4101b745b..b0852affa 100644 --- a/test/build-exec-environment.spec.js +++ b/test/build-exec-environment.spec.js @@ -64,6 +64,27 @@ describe('build-exec-environment', () => { 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 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 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 env = buildEnvironment(ctx); + expect(env.NO_COLOR).to.equal('1'); + }); }); describe('synthetic vars', () => { @@ -87,13 +108,6 @@ describe('build-exec-environment', () => { // CLICOLOR_FORCE affects all streams, not just stderr. expect(env).to.not.have.property('CLICOLOR_FORCE'); }); - - it('should not override inherited CLICOLOR_FORCE with synthetic', () => { - 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'); - }); }); describe('user overrides', () => { diff --git a/utils/build-exec-environment.js b/utils/build-exec-environment.js index c95034e1b..04c20b490 100644 --- a/utils/build-exec-environment.js +++ b/utils/build-exec-environment.js @@ -12,6 +12,8 @@ const forwardKeys = [ 'DEBUG', 'VERBOSE', ]; +const forceColorKeys = ['FORCE_COLOR', 'CLICOLOR_FORCE']; + /* * Builds the environment variables for a docker exec invocation. * @@ -23,7 +25,13 @@ const forwardKeys = [ 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) 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]; } const synthetic = {}; From da3b7513372a2d471e04dadbf2fec2ae8e626d65 Mon Sep 17 00:00:00 2001 From: Aaron Feledy Date: Fri, 10 Apr 2026 20:15:17 -0500 Subject: [PATCH 13/17] refactor: thread env through context for buildEnvironment 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. --- test/build-exec-environment.spec.js | 97 ++++++++++++++++------------- test/describe-context.spec.js | 8 ++- utils/build-exec-environment.js | 5 +- utils/describe-context.js | 1 + 4 files changed, 63 insertions(+), 48 deletions(-) diff --git a/test/build-exec-environment.spec.js b/test/build-exec-environment.spec.js index b0852affa..4ca000e8f 100644 --- a/test/build-exec-environment.spec.js +++ b/test/build-exec-environment.spec.js @@ -11,121 +11,127 @@ chai.should(); const buildEnvironment = require('../utils/build-exec-environment'); -describe('build-exec-environment', () => { - const savedEnv = {}; - - beforeEach(() => { - // Save and clear forwarded env vars to isolate tests - for (const key of buildEnvironment.forwardKeys) { - savedEnv[key] = process.env[key]; - delete process.env[key]; - } - }); - - afterEach(() => { - // Restore env vars - for (const key of buildEnvironment.forwardKeys) { - if (savedEnv[key] !== undefined) process.env[key] = savedEnv[key]; - else delete process.env[key]; - } - }); +// 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; + return { + stdout: {isTTY: true, columns: 80, rows: 24, ...stdout}, + stderr: {isTTY: true, ...stderr}, + env: env || {}, + noColor: false, + ...rest, + }; +}; +describe('build-exec-environment', () => { 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 = makeCtx({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 = makeCtx(); 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 = makeCtx({env: {CI: 'true'}, stdout: {isTTY: false, columns: 80, rows: 24}}); 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 = makeCtx({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 = makeCtx({env: {TZ: 'America/New_York'}}); const env = buildEnvironment(ctx); expect(env.TZ).to.equal('America/New_York'); }); + it('should forward FORCE_COLOR when stdout is a TTY', () => { + const ctx = makeCtx({env: {FORCE_COLOR: '1'}}); + const env = buildEnvironment(ctx); + expect(env.FORCE_COLOR).to.equal('1'); + }); + 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 = makeCtx({env: {FORCE_COLOR: '1'}, stdout: {isTTY: false, columns: 80, rows: 24}}); 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 = makeCtx({env: {CLICOLOR_FORCE: '3'}, stdout: {isTTY: false, columns: 80, rows: 24}}); 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 = makeCtx({env: {NO_COLOR: '1'}, stdout: {isTTY: false, columns: 80, rows: 24}, noColor: true}); const env = buildEnvironment(ctx); expect(env.NO_COLOR).to.equal('1'); }); + + it('should ignore env vars not in forwardKeys', () => { + const ctx = makeCtx({env: {TERM: 'xterm', SECRET_TOKEN: 'abc123'}}); + const env = buildEnvironment(ctx); + expect(env.TERM).to.equal('xterm'); + expect(env).to.not.have.property('SECRET_TOKEN'); + }); }); 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 = makeCtx({stdout: {isTTY: false, 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 stdout is a TTY', () => { - const ctx = {stdout: {isTTY: true, columns: 120, rows: 40}, stderr: {isTTY: true}, noColor: false}; + const ctx = makeCtx({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'); }); 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 = makeCtx({stdout: {isTTY: false, columns: 80, rows: 24}, stderr: {isTTY: true}}); const env = buildEnvironment(ctx); - // CLICOLOR_FORCE affects all streams, not just stderr. expect(env).to.not.have.property('CLICOLOR_FORCE'); }); }); 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 = makeCtx({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 = makeCtx({stdout: {isTTY: false, columns: 80, rows: 24}}); const env = buildEnvironment(ctx, {COLUMNS: '200'}); expect(env.COLUMNS).to.equal('200'); }); + it('should let user env force color vars even when stdout is not a TTY', () => { + const ctx = makeCtx({env: {FORCE_COLOR: '1'}, stdout: {isTTY: false, columns: 80, rows: 24}}); + const env = buildEnvironment(ctx, {FORCE_COLOR: '3'}); + // Inherited FORCE_COLOR is skipped, but explicit user override wins + expect(env.FORCE_COLOR).to.equal('3'); + }); + it('should pass through arbitrary user vars', () => { - const ctx = {stdout: {isTTY: true}, stderr: {isTTY: true}, noColor: false}; + const ctx = makeCtx(); const env = buildEnvironment(ctx, {MY_APP_VAR: 'hello'}); expect(env.MY_APP_VAR).to.equal('hello'); }); @@ -133,12 +139,13 @@ describe('build-exec-environment', () => { 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 env = buildEnvironment(ctx, {COLUMNS: '999', FORCE_COLOR: '3'}); + const env = buildEnvironment( + makeCtx({stdout: {isTTY: false, columns: 80, rows: 24}}), + {COLUMNS: '999', FORCE_COLOR: '3'}, + ); // User wins over synthetic expect(env.COLUMNS).to.equal('999'); - // User wins over inherited + // User wins (FORCE_COLOR not inherited because !isTTY, but user sets it) expect(env.FORCE_COLOR).to.equal('3'); }); }); diff --git a/test/describe-context.spec.js b/test/describe-context.spec.js index 0ea83146b..5344d2a19 100644 --- a/test/describe-context.spec.js +++ b/test/describe-context.spec.js @@ -32,17 +32,23 @@ describe('describe-context', () => { 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, stderr, env, and flags', () => { 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('env'); expect(ctx).to.have.property('isNodeMode'); expect(ctx).to.have.property('ci'); expect(ctx).to.have.property('noColor'); expect(ctx).to.have.property('forceColor'); }); + it('should expose process.env as env', () => { + const ctx = describeContext(); + expect(ctx.env).to.equal(process.env); + }); + it('should reflect stdin TTY state', () => { process.stdin.isTTY = true; expect(describeContext().stdin.isTTY).to.be.true; diff --git a/utils/build-exec-environment.js b/utils/build-exec-environment.js index 04c20b490..50d542cd6 100644 --- a/utils/build-exec-environment.js +++ b/utils/build-exec-environment.js @@ -23,15 +23,16 @@ const forceColorKeys = ['FORCE_COLOR', 'CLICOLOR_FORCE']; * 3. userEnv — explicit user overrides (always win) */ module.exports = (context, userEnv = {}) => { + const hostEnv = context.env || process.env; const inherited = {}; for (const key of forwardKeys) { - if (process.env[key] === undefined) continue; + if (hostEnv[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] = hostEnv[key]; } const synthetic = {}; diff --git a/utils/describe-context.js b/utils/describe-context.js index e742091f4..84b3a2979 100644 --- a/utils/describe-context.js +++ b/utils/describe-context.js @@ -20,6 +20,7 @@ module.exports = () => ({ stderr: { isTTY: Boolean(process.stderr.isTTY), }, + env: process.env, isNodeMode: process.lando === 'node', ci: Boolean(process.env.CI), noColor: process.env.NO_COLOR !== undefined, From a0b832d66456e668239cf071989d100bc5b76e01 Mon Sep 17 00:00:00 2001 From: Aaron Feledy Date: Tue, 14 Apr 2026 12:47:20 -0500 Subject: [PATCH 14/17] fix: derive container color env from chalk, inject COLUMNS/LINES in compose path, preserve datum.cmd mutation contract --- lib/compose.js | 16 ++++- test/build-exec-environment.spec.js | 94 +++++++++++++++++++---------- test/describe-context.spec.js | 34 ++--------- test/tty-allocation.spec.js | 74 ++++++++++++++++++++++- utils/build-docker-exec.js | 7 +++ utils/build-exec-environment.js | 27 ++++----- utils/describe-context.js | 10 ++- 7 files changed, 182 insertions(+), 80 deletions(-) diff --git a/lib/compose.js b/lib/compose.js index 186f97dbc..9f430607a 100644 --- a/lib/compose.js +++ b/lib/compose.js @@ -133,15 +133,29 @@ exports.run = (compose, project, opts = {}) => { } } + const context = describeContext(); + // Detached execs should never allocate a TTY. Otherwise compute TTY // at call time so the decision reflects the current terminal state. if (opts.detach === true) { opts.noTTY = true; } else if (opts.noTTY === undefined) { - const context = describeContext(); 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'; + } + opts.environment = {...envDefaults, ...(opts.environment || {})}; + return buildShell('exec', project, compose, opts); }; diff --git a/test/build-exec-environment.spec.js b/test/build-exec-environment.spec.js index 4ca000e8f..76a5933e6 100644 --- a/test/build-exec-environment.spec.js +++ b/test/build-exec-environment.spec.js @@ -19,7 +19,7 @@ const makeCtx = (overrides = {}) => { stdout: {isTTY: true, columns: 80, rows: 24, ...stdout}, stderr: {isTTY: true, ...stderr}, env: env || {}, - noColor: false, + landoColorLevel: 3, ...rest, }; }; @@ -38,16 +38,16 @@ describe('build-exec-environment', () => { expect(env).to.not.have.property('TERM'); }); - it('should forward CI when set', () => { - const ctx = makeCtx({env: {CI: 'true'}, stdout: {isTTY: false, columns: 80, rows: 24}}); + it('should forward COLORTERM when set', () => { + const ctx = makeCtx({env: {COLORTERM: 'truecolor'}}); const env = buildEnvironment(ctx); - expect(env.CI).to.equal('true'); + expect(env.COLORTERM).to.equal('truecolor'); }); - it('should forward DEBUG when set', () => { - const ctx = makeCtx({env: {DEBUG: '*'}}); + it('should forward TERM_PROGRAM when set', () => { + const ctx = makeCtx({env: {TERM_PROGRAM: 'iTerm.app'}}); const env = buildEnvironment(ctx); - expect(env.DEBUG).to.equal('*'); + expect(env.TERM_PROGRAM).to.equal('iTerm.app'); }); it('should forward TZ when set', () => { @@ -56,35 +56,64 @@ describe('build-exec-environment', () => { expect(env.TZ).to.equal('America/New_York'); }); - it('should forward FORCE_COLOR when stdout is a TTY', () => { - const ctx = makeCtx({env: {FORCE_COLOR: '1'}}); + it('should forward locale vars when set', () => { + const ctx = makeCtx({env: {LANG: 'en_US.UTF-8', LC_ALL: 'C', LC_CTYPE: 'UTF-8', LC_MESSAGES: 'en_US'}}); const env = buildEnvironment(ctx); - expect(env.FORCE_COLOR).to.equal('1'); + expect(env.LANG).to.equal('en_US.UTF-8'); + expect(env.LC_ALL).to.equal('C'); + expect(env.LC_CTYPE).to.equal('UTF-8'); + expect(env.LC_MESSAGES).to.equal('en_US'); }); - it('should not forward FORCE_COLOR when stdout is not a TTY', () => { - const ctx = makeCtx({env: {FORCE_COLOR: '1'}, stdout: {isTTY: false, columns: 80, rows: 24}}); + it('should ignore env vars not in forwardKeys', () => { + const ctx = makeCtx({env: {TERM: 'xterm', SECRET_TOKEN: 'abc123'}}); const env = buildEnvironment(ctx); - expect(env).to.not.have.property('FORCE_COLOR'); + expect(env.TERM).to.equal('xterm'); + expect(env).to.not.have.property('SECRET_TOKEN'); + }); + + it('should not forward CI env vars', () => { + const ctx = makeCtx({env: {CI: 'true', GITHUB_ACTIONS: 'true', GITLAB_CI: 'true'}}); + const env = buildEnvironment(ctx); + expect(env).to.not.have.property('CI'); + expect(env).to.not.have.property('GITHUB_ACTIONS'); + expect(env).to.not.have.property('GITLAB_CI'); + }); + + it('should not forward DEBUG or VERBOSE', () => { + const ctx = makeCtx({env: {DEBUG: '*', VERBOSE: '1'}}); + const env = buildEnvironment(ctx); + expect(env).to.not.have.property('DEBUG'); + expect(env).to.not.have.property('VERBOSE'); }); - it('should not forward CLICOLOR_FORCE when stdout is not a TTY', () => { - const ctx = makeCtx({env: {CLICOLOR_FORCE: '3'}, stdout: {isTTY: false, columns: 80, rows: 24}}); + it('should not forward color env vars from the host', () => { + const ctx = makeCtx({env: {FORCE_COLOR: '3', NO_COLOR: '1', CLICOLOR: '1', CLICOLOR_FORCE: '1'}}); const env = buildEnvironment(ctx); + // Color state is derived from Lando's own chalk level, not host vars + expect(env).to.not.have.property('FORCE_COLOR'); + expect(env).to.not.have.property('CLICOLOR'); expect(env).to.not.have.property('CLICOLOR_FORCE'); }); + }); - it('should still forward NO_COLOR when stdout is not a TTY', () => { - const ctx = makeCtx({env: {NO_COLOR: '1'}, stdout: {isTTY: false, columns: 80, rows: 24}, noColor: true}); + describe('color suppression from Lando state', () => { + it('should set NO_COLOR=1 when Lando is not producing color', () => { + const ctx = makeCtx({landoColorLevel: 0}); const env = buildEnvironment(ctx); expect(env.NO_COLOR).to.equal('1'); }); - it('should ignore env vars not in forwardKeys', () => { - const ctx = makeCtx({env: {TERM: 'xterm', SECRET_TOKEN: 'abc123'}}); + it('should not set NO_COLOR when Lando is producing color', () => { + const ctx = makeCtx({landoColorLevel: 3}); const env = buildEnvironment(ctx); - expect(env.TERM).to.equal('xterm'); - expect(env).to.not.have.property('SECRET_TOKEN'); + expect(env).to.not.have.property('NO_COLOR'); + }); + + it('should not set NO_COLOR when Lando has basic color support', () => { + const ctx = makeCtx({landoColorLevel: 1}); + const env = buildEnvironment(ctx); + expect(env).to.not.have.property('NO_COLOR'); }); }); @@ -102,12 +131,6 @@ describe('build-exec-environment', () => { 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 = makeCtx({stdout: {isTTY: false, columns: 80, rows: 24}, stderr: {isTTY: true}}); - const env = buildEnvironment(ctx); - expect(env).to.not.have.property('CLICOLOR_FORCE'); - }); }); describe('user overrides', () => { @@ -123,10 +146,17 @@ describe('build-exec-environment', () => { expect(env.COLUMNS).to.equal('200'); }); - it('should let user env force color vars even when stdout is not a TTY', () => { - const ctx = makeCtx({env: {FORCE_COLOR: '1'}, stdout: {isTTY: false, columns: 80, rows: 24}}); + it('should let user env override NO_COLOR suppression', () => { + const ctx = makeCtx({landoColorLevel: 0}); + const env = buildEnvironment(ctx, {NO_COLOR: ''}); + // User explicitly clearing NO_COLOR should win + expect(env.NO_COLOR).to.equal(''); + }); + + it('should let user env force color even when Lando has no color', () => { + const ctx = makeCtx({landoColorLevel: 0}); const env = buildEnvironment(ctx, {FORCE_COLOR: '3'}); - // Inherited FORCE_COLOR is skipped, but explicit user override wins + // Synthetic NO_COLOR is set, but user FORCE_COLOR is also present expect(env.FORCE_COLOR).to.equal('3'); }); @@ -141,12 +171,10 @@ describe('build-exec-environment', () => { it('should apply inherited < synthetic < user', () => { const env = buildEnvironment( makeCtx({stdout: {isTTY: false, columns: 80, rows: 24}}), - {COLUMNS: '999', FORCE_COLOR: '3'}, + {COLUMNS: '999'}, ); // User wins over synthetic expect(env.COLUMNS).to.equal('999'); - // User wins (FORCE_COLOR not inherited because !isTTY, but user sets it) - expect(env.FORCE_COLOR).to.equal('3'); }); }); }); diff --git a/test/describe-context.spec.js b/test/describe-context.spec.js index 5344d2a19..78be9a071 100644 --- a/test/describe-context.spec.js +++ b/test/describe-context.spec.js @@ -25,11 +25,7 @@ describe('describe-context', () => { 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, env, and flags', () => { @@ -40,8 +36,7 @@ describe('describe-context', () => { expect(ctx).to.have.property('env'); 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('landoColorLevel'); }); it('should expose process.env as env', () => { @@ -101,27 +96,10 @@ describe('describe-context', () => { 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; + it('should expose landoColorLevel as a number from chalk', () => { + const ctx = describeContext(); + expect(ctx.landoColorLevel).to.be.a('number'); + expect(ctx.landoColorLevel).to.be.at.least(0); + expect(ctx.landoColorLevel).to.be.at.most(3); }); }); diff --git a/test/tty-allocation.spec.js b/test/tty-allocation.spec.js index 5c751442a..b284432e4 100644 --- a/test/tty-allocation.spec.js +++ b/test/tty-allocation.spec.js @@ -29,8 +29,7 @@ const makeContext = (overrides = {}) => { stderr: {isTTY: false, ...stderr}, isNodeMode: true, ci: false, - noColor: false, - forceColor: undefined, + landoColorLevel: 0, ...rest, }; }; @@ -168,6 +167,29 @@ describe('TTY allocation', () => { }); }); + describe('datum mutation (build-docker-exec.js caller contract)', () => { + it('should write cleaned cmd back to datum so compose fallback sees it', () => { + const ctx = makeContext(); + const datum = makeDatum({cmd: ['sleep', '100', '&']}); + // Simulate what the exported module function does + 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('&'); + }); + + it('should write cleaned cmd for shell wrapper detach', () => { + 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']); + }); + }); + describe('compose exec (lib/compose.js)', () => { const originalStdinIsTTY = process.stdin.isTTY; const originalStdoutIsTTY = process.stdout.isTTY; @@ -249,5 +271,53 @@ describe('TTY allocation', () => { expect(result.cmd).to.include('--detach'); expect(result.cmd).to.include('-T'); }); + + it('should inject COLUMNS and LINES when noTTY is set', () => { + process.stdin.isTTY = false; + process.stdout.isTTY = false; + const compose = require('../lib/compose'); + const result = compose.run( + ['docker-compose.yml'], + 'test_project', + {services: ['web'], cmd: ['echo', 'hello']}, + ); + expect(result.cmd).to.include('-T'); + // COLUMNS and LINES should be injected via --env + expect(result.cmd).to.include('--env'); + const envPairs = result.cmd.filter((_, i, a) => i > 0 && a[i - 1] === '--env'); + const colEntry = envPairs.find(e => e.startsWith('COLUMNS=')); + const lineEntry = envPairs.find(e => e.startsWith('LINES=')); + expect(colEntry).to.be.a('string'); + expect(lineEntry).to.be.a('string'); + }); + + it('should not inject COLUMNS and LINES when TTY is allocated', () => { + process.stdin.isTTY = true; + process.stdout.isTTY = true; + const compose = require('../lib/compose'); + const result = compose.run( + ['docker-compose.yml'], + 'test_project', + {services: ['web'], cmd: ['echo', 'hello']}, + ); + const envPairs = result.cmd.filter((_, i, a) => i > 0 && a[i - 1] === '--env'); + const colEntry = envPairs.find(e => e.startsWith('COLUMNS=')); + expect(colEntry).to.be.undefined; + }); + + it('should not clobber caller-provided environment vars', () => { + process.stdin.isTTY = false; + process.stdout.isTTY = false; + const compose = require('../lib/compose'); + const result = compose.run( + ['docker-compose.yml'], + 'test_project', + {services: ['web'], cmd: ['echo', 'hello'], environment: {COLUMNS: '999', MY_VAR: 'keep'}}, + ); + const envPairs = result.cmd.filter((_, i, a) => i > 0 && a[i - 1] === '--env'); + // Caller COLUMNS should win over the injected default + expect(envPairs).to.include('COLUMNS=999'); + expect(envPairs).to.include('MY_VAR=keep'); + }); }); }); diff --git a/utils/build-docker-exec.js b/utils/build-docker-exec.js index 4ae233b85..8ec325d37 100644 --- a/utils/build-docker-exec.js +++ b/utils/build-docker-exec.js @@ -53,6 +53,13 @@ module.exports = (injected, stdio, datum = {}) => { const dockerBin = injected.config.dockerBin || injected._config.dockerBin; const context = describeContext(); const args = 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; + return injected.shell.sh(args, {mode: 'attach', cstdio: stdio}); }; diff --git a/utils/build-exec-environment.js b/utils/build-exec-environment.js index 50d542cd6..3af0ff586 100644 --- a/utils/build-exec-environment.js +++ b/utils/build-exec-environment.js @@ -1,25 +1,21 @@ 'use strict'; -// Host variables to forward when set. These provide terminal, locale, -// and CI context to whatever runs inside the container. +// Host variables to forward when set. Terminal type and locale +// provide context so tools inside the container produce appropriate +// output; TZ keeps timestamps consistent with the host. const forwardKeys = [ 'TERM', 'COLORTERM', 'TERM_PROGRAM', - 'NO_COLOR', 'FORCE_COLOR', 'CLICOLOR', 'CLICOLOR_FORCE', 'LANG', 'LC_ALL', 'LC_CTYPE', 'LC_MESSAGES', 'TZ', - 'CI', 'GITHUB_ACTIONS', 'GITLAB_CI', 'CIRCLECI', - 'BUILDKITE', 'JENKINS_URL', 'TRAVIS', - 'DEBUG', 'VERBOSE', ]; -const forceColorKeys = ['FORCE_COLOR', 'CLICOLOR_FORCE']; - /* * Builds the environment variables for a docker exec invocation. * * Three layers with explicit precedence: * 1. inherited — host vars forwarded when set - * 2. synthetic — derived from context analysis (e.g. COLUMNS/LINES) + * 2. synthetic — derived from context analysis (e.g. COLUMNS/LINES, + * color suppression when Lando itself is not producing color) * 3. userEnv — explicit user overrides (always win) */ module.exports = (context, userEnv = {}) => { @@ -27,16 +23,19 @@ module.exports = (context, userEnv = {}) => { const inherited = {}; for (const key of forwardKeys) { if (hostEnv[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] = hostEnv[key]; } const synthetic = {}; + // When Lando itself isn't producing colorful output, tell containers + // not to either. context.landoColorLevel mirrors chalk.level which + // already accounts for NO_COLOR, FORCE_COLOR, TERM=dumb, TTY state, + // and every other signal the host uses to decide on color support. + if (context.landoColorLevel === 0) { + synthetic.NO_COLOR = '1'; + } + if (!context.stdout.isTTY) { // No PTY means no SIGWINCH, but a static hint is better than nothing synthetic.COLUMNS = String(context.stdout.columns); diff --git a/utils/describe-context.js b/utils/describe-context.js index 84b3a2979..eaa22d083 100644 --- a/utils/describe-context.js +++ b/utils/describe-context.js @@ -1,5 +1,7 @@ 'use strict'; +const chalk = require('chalk'); + /* * Describes the current execution context as a plain object. * @@ -23,6 +25,10 @@ module.exports = () => ({ env: process.env, isNodeMode: process.lando === 'node', ci: Boolean(process.env.CI), - noColor: process.env.NO_COLOR !== undefined, - forceColor: process.env.FORCE_COLOR, + // chalk.level: 0=none, 1=basic, 2=256, 3=truecolor. + // Reflects whether Lando itself is producing colorful output. + // chalk already accounts for NO_COLOR, FORCE_COLOR, TERM=dumb, + // TTY state, etc. so downstream code can treat this as the single + // source of truth for color support. + landoColorLevel: chalk.level, }); From 8609ced50ece2a518b1f9ed8c7cb99bc70ddfa83 Mon Sep 17 00:00:00 2001 From: Aaron Feledy Date: Tue, 14 Apr 2026 15:35:05 -0500 Subject: [PATCH 15/17] chore: add JSDoc type definitions for exec pipeline --- lib/compose.types.js | 31 ++++++++++++++++++++++++ utils/build-docker-exec.types.js | 21 ++++++++++++++++ utils/describe-context.types.js | 41 ++++++++++++++++++++++++++++++++ utils/extract-detach.types.js | 13 ++++++++++ 4 files changed, 106 insertions(+) create mode 100644 lib/compose.types.js create mode 100644 utils/build-docker-exec.types.js create mode 100644 utils/describe-context.types.js create mode 100644 utils/extract-detach.types.js diff --git a/lib/compose.types.js b/lib/compose.types.js new file mode 100644 index 000000000..26c2d5cfe --- /dev/null +++ b/lib/compose.types.js @@ -0,0 +1,31 @@ +'use strict'; + +/** + * @file Type definitions for {@link module:lib/compose}. + */ + +/** + * @typedef {object} ComposeRunOpts + * @property {boolean} [detach] — run in detached mode + * @property {string[]} [cmd] — command array to exec in the container + * @property {boolean} [noTTY] — disable PTY allocation (computed from + * terminal state when not set explicitly) + * @property {Record} [environment] — env vars to inject + * @property {string[]} [services] — target service name(s) + * @property {string} [user] — user to run as inside the container + * @property {string} [workdir] — working directory inside the container + * @property {string[]} [entrypoint] — override container entrypoint + * @property {string} [cstdio] — child stdio mode + * @property {boolean} [silent] — suppress output + */ + +/** + * @typedef {object} ShellSpec + * @property {string[]} cmd — the assembled docker-compose argv + * @property {object} opts — options passed to shell.sh + * @property {string} opts.mode — shell execution mode + * @property {string} [opts.cstdio] — child stdio mode + * @property {boolean} [opts.silent] — suppress output + */ + +module.exports = {}; diff --git a/utils/build-docker-exec.types.js b/utils/build-docker-exec.types.js new file mode 100644 index 000000000..360769909 --- /dev/null +++ b/utils/build-docker-exec.types.js @@ -0,0 +1,21 @@ +'use strict'; + +/** + * @file Type definitions for {@link module:utils/build-docker-exec}. + */ + +/** + * @typedef {object} ExecDatumOpts + * @property {string} [workdir] — working directory inside the container + * @property {string} user — user to run as inside the container + * @property {Record} [environment] — caller-provided env overrides + */ + +/** + * @typedef {object} ExecDatum + * @property {string[]} cmd — the command to execute + * @property {string} id — target container id + * @property {ExecDatumOpts} opts + */ + +module.exports = {}; diff --git a/utils/describe-context.types.js b/utils/describe-context.types.js new file mode 100644 index 000000000..1055673e7 --- /dev/null +++ b/utils/describe-context.types.js @@ -0,0 +1,41 @@ +'use strict'; + +/** + * @file Type definitions for {@link module:utils/describe-context}. + */ + +/** + * @typedef {object} StdinContext + * @property {boolean} isTTY — whether stdin is connected to a terminal + * @property {boolean} isClosed — whether the readable side has already ended + */ + +/** + * @typedef {object} StdoutContext + * @property {boolean} isTTY — whether stdout is connected to a terminal + * @property {number} columns — terminal width (defaults to 80 when not a TTY) + * @property {number} rows — terminal height (defaults to 24 when not a TTY) + */ + +/** + * @typedef {object} StderrContext + * @property {boolean} isTTY — whether stderr is connected to a terminal + */ + +/** + * @typedef {object} ExecutionContext + * @property {StdinContext} stdin + * @property {StdoutContext} stdout + * @property {StderrContext} stderr + * @property {Record} env — host environment variables + * @property {boolean} isNodeMode — true when `process.lando === 'node'` + * @property {boolean} ci — true when the CI env var is set + * @property {0 | 1 | 2 | 3} landoColorLevel — chalk color level + * (0 = none, 1 = basic, 2 = 256-color, 3 = truecolor). + * Reflects whether Lando itself is producing colorful output. + * chalk already accounts for NO_COLOR, FORCE_COLOR, TERM=dumb, + * TTY state, etc. so downstream code can treat this as the single + * source of truth for color support. + */ + +module.exports = {}; diff --git a/utils/extract-detach.types.js b/utils/extract-detach.types.js new file mode 100644 index 000000000..b7e2f5b25 --- /dev/null +++ b/utils/extract-detach.types.js @@ -0,0 +1,13 @@ +'use strict'; + +/** + * @file Type definitions for {@link module:utils/extract-detach}. + */ + +/** + * @typedef {object} DetachResult + * @property {string[]} cmd — cleaned command array with detach syntax removed + * @property {boolean} detach — whether detach intent was detected + */ + +module.exports = {}; From d4fc79170e1db0d593dd0de8583cd6a3f998ef48 Mon Sep 17 00:00:00 2001 From: Aaron Feledy Date: Tue, 14 Apr 2026 22:08:16 -0500 Subject: [PATCH 16/17] refactor: deduplicate compose exec environment building Replace hand-rolled COLUMNS/LINES and NO_COLOR defaults in compose.run() with shared buildEnvironment() utility from build-exec-environment.js. Both exec paths now go through the same function, ensuring consistent host terminal environment forwarding (TERM, LANG, TZ, etc.). Also add CHANGELOG entries for the new environment forwarding and NO_COLOR injection behaviors. --- CHANGELOG.md | 5 ++++- lib/compose.js | 17 +++++------------ 2 files changed, 9 insertions(+), 13 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c80925133..cd0f34861 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,11 +1,14 @@ ## {{ UNRELEASED_VERSION }} - [{{ UNRELEASED_DATE }}]({{ UNRELEASED_LINK }}) +* Fixed ANSI escape codes appearing in redirected output by checking `stdout.isTTY` instead of `stdin.isTTY` for TTY allocation [#345](https://github.com/lando/core/issues/345) +* Improved exec and tooling commands to forward host terminal environment (`TERM`, `LANG`, `TZ`, etc.) into containers +* Improved color output handling so containers receive `NO_COLOR=1` when Lando itself is running without color + ## v3.26.3 - [April 14, 2026](https://github.com/lando/core/releases/tag/v3.26.3) * Fixed crash when `config.yml` is empty instead of containing `{}` [#439](https://github.com/lando/core/issues/439) * Updated `lando setup` to install `docker-buildx` if missing * Fixed `buildx` built images not being available locally by adding `--load` flag -* Fixed ANSI escape codes appearing in redirected output by checking `stdout.isTTY` instead of `stdin.isTTY` for TTY allocation [#345](https://github.com/lando/core/issues/345) ## v3.26.2 - [December 17, 2025](https://github.com/lando/core/releases/tag/v3.26.2) diff --git a/lib/compose.js b/lib/compose.js index 9f430607a..83374b6ff 100644 --- a/lib/compose.js +++ b/lib/compose.js @@ -2,6 +2,7 @@ // Modules const _ = require('lodash'); +const buildEnvironment = require('../utils/build-exec-environment'); const describeContext = require('../utils/describe-context'); const extractDetach = require('../utils/extract-detach'); @@ -143,18 +144,10 @@ exports.run = (compose, project, opts = {}) => { 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'; - } - opts.environment = {...envDefaults, ...(opts.environment || {})}; + // Build the environment using the same shared utility as the docker + // exec path so both code paths forward identical host hints (TERM, + // LANG, COLUMNS/LINES, NO_COLOR, etc.). Caller-provided vars always win. + opts.environment = buildEnvironment(context, opts.environment || {}); return buildShell('exec', project, compose, opts); }; From 5606d581671d37298c0d0956747d228982dd16fe Mon Sep 17 00:00:00 2001 From: Aaron Feledy Date: Wed, 15 Apr 2026 02:10:52 -0500 Subject: [PATCH 17/17] fix: add missing env to test helper, deduplicate extractDetach call --- test/tty-allocation.spec.js | 3 ++- utils/build-docker-exec.js | 6 +++++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/test/tty-allocation.spec.js b/test/tty-allocation.spec.js index b284432e4..f05232711 100644 --- a/test/tty-allocation.spec.js +++ b/test/tty-allocation.spec.js @@ -21,12 +21,13 @@ const {buildExecArgs} = require('../utils/build-docker-exec'); // Helper to build a minimal context object for testing const makeContext = (overrides = {}) => { - const {stdin, stdout, stderr, ...rest} = overrides; + const {stdin, stdout, stderr, env, ...rest} = overrides; return { stdin: {isTTY: false, isClosed: false, ...stdin}, stdout: {isTTY: false, columns: 80, rows: 24, ...stdout}, stderr: {isTTY: false, ...stderr}, + env: env || {}, isNodeMode: true, ci: false, landoColorLevel: 0, diff --git a/utils/build-docker-exec.js b/utils/build-docker-exec.js index 8ec325d37..30f72e8da 100644 --- a/utils/build-docker-exec.js +++ b/utils/build-docker-exec.js @@ -52,13 +52,17 @@ const buildExecArgs = (docker, datum, context) => { module.exports = (injected, stdio, datum = {}) => { const dockerBin = injected.config.dockerBin || injected._config.dockerBin; const context = describeContext(); + + // Extract detach once and reuse the cleaned command for both + // the arg builder and the datum mutation contract. + const {cmd: cleanCmd} = extractDetach(datum.cmd); const args = 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 = cleanCmd; return injected.shell.sh(args, {mode: 'attach', cstdio: stdio}); };