diff --git a/ci/test.sh b/ci/test.sh index 88448cb6..1d78ac95 100755 --- a/ci/test.sh +++ b/ci/test.sh @@ -37,7 +37,10 @@ echo "==> dart test (pty — unreliable under the flutter test runner; serial)" # for resource-bound tests, vs. the old per-test `retry:` band-aid. (T-193) # windows_pty_test is the ConPTY sibling of session_test; each suite # self-skips off-platform, so the union always contributes tests. -dart test -r "$REPORTER" --concurrency=1 --tags pty test/pty/session_test.dart test/panes/registry_test.dart test/pty/windows_pty_test.dart +# --timeout 60s matches the flutter lines below: a wedged PTY test (e.g. a +# ConPTY reader blocked forever in ReadFile) fails fast instead of hanging the +# whole serial run. +dart test -r "$REPORTER" --concurrency=1 --timeout 60s --tags pty test/pty/session_test.dart test/panes/registry_test.dart test/pty/windows_pty_test.dart # The parallel pool excludes both pty (runs under dart test, above) and # serial-tagged tests (concurrency-vulnerable — run in their own --concurrency=1 diff --git a/lib/src/pty/native_pty.dart b/lib/src/pty/native_pty.dart index 6bd7f21c..3043760e 100644 --- a/lib/src/pty/native_pty.dart +++ b/lib/src/pty/native_pty.dart @@ -25,6 +25,7 @@ import 'dart:typed_data'; import 'package:ffi/ffi.dart'; import 'errors.dart'; +import 'pty_size.dart'; import '../ipc/errno_mapping.dart' show PosixErrno; import 'ffi/libc.dart' as libc; import 'pty_session.dart'; @@ -310,8 +311,8 @@ class NativePty implements PtySession { // ---- Set initial winsize on the master --------------------------- final ws = calloc<_Winsize>() - ..ref.wsRow = rows - ..ref.wsCol = columns; + ..ref.wsRow = clampPtyDimension(rows) + ..ref.wsCol = clampPtyDimension(columns); _ioctl(masterFd, _kTiocsWinsz, ws); calloc.free(ws); @@ -429,8 +430,8 @@ class NativePty implements PtySession { void resize({required int cols, required int rows}) { if (_dead) return; final ws = calloc<_Winsize>() - ..ref.wsRow = rows - ..ref.wsCol = cols; + ..ref.wsRow = clampPtyDimension(rows) + ..ref.wsCol = clampPtyDimension(cols); final rc = _ioctl(_fd, _kTiocsWinsz, ws); calloc.free(ws); if (rc < 0 && libc.errno == PosixErrno.ebadf) { diff --git a/lib/src/pty/pty_size.dart b/lib/src/pty/pty_size.dart new file mode 100644 index 00000000..0764f8b5 --- /dev/null +++ b/lib/src/pty/pty_size.dart @@ -0,0 +1,15 @@ +/// Minimum dimension handed to any PTY backend. +/// +/// A 1-column ConPTY makes the Windows conhost spin emitting CRLF forever +/// (microsoft/terminal#19922), and a 0 in either axis is invalid on both +/// platforms. Every backend clamps its spawn + resize through this, so a +/// degenerate size from the UI — a pane measured at zero width during a +/// transient layout pass — can never wedge a child. The floor (2) is below +/// any real terminal, so the clamp is invisible in normal use. +library; + +/// Smallest column/row count a PTY backend will accept. +const int minPtyDimension = 2; + +/// Clamp a column or row count up to [minPtyDimension]. +int clampPtyDimension(int value) => value < minPtyDimension ? minPtyDimension : value; diff --git a/lib/src/pty/windows_pty.dart b/lib/src/pty/windows_pty.dart index fb88dd88..04b97539 100644 --- a/lib/src/pty/windows_pty.dart +++ b/lib/src/pty/windows_pty.dart @@ -46,6 +46,7 @@ import 'package:ffi/ffi.dart'; import 'errors.dart'; import 'pty_session.dart'; +import 'pty_size.dart'; // -- structs ---------------------------------------------------------------- @@ -248,7 +249,7 @@ class WindowsPty implements PtySession { String? workingDirectory, Map environment = const {}, }) { - executable = _resolveExecutable(executable, environment); + executable = resolveExecutable(executable, environment); // ---- Pipes + pseudo console --------------------------------------- final ha = calloc<_Handle>(); @@ -275,8 +276,8 @@ class WindowsPty implements PtySession { calloc.free(hb); final size = calloc<_Coord>() - ..ref.x = columns - ..ref.y = rows; + ..ref.x = clampPtyDimension(columns) + ..ref.y = clampPtyDimension(rows); final hpcOut = calloc<_Handle>(); final hr = _createPseudoConsole(size.ref, inRead, outWrite, 0, hpcOut); calloc.free(size); @@ -332,8 +333,8 @@ class WindowsPty implements PtySession { // parse (which also gives .bat/.cmd their cmd.exe host); the // executable is pre-resolved to an absolute path above so no PATH // ambiguity is left at this point. - final cmdLine = [executable, ...arguments].map(_quoteArg).join(' ').toNativeUtf16(allocator: malloc); - final envBlock = _environmentBlock(environment); + final cmdLine = [executable, ...arguments].map(quoteArg).join(' ').toNativeUtf16(allocator: malloc); + final envBlock = composeEnvironmentBlock(environment).toNativeUtf16(allocator: malloc); final cwdN = workingDirectory == null ? ffi.nullptr : workingDirectory.toNativeUtf16(allocator: malloc); // STARTF_USESTDHANDLES with NULL std handles (calloc zeroes them): @@ -508,8 +509,8 @@ class WindowsPty implements PtySession { void resize({required int cols, required int rows}) { if (_dead || _hpc == ffi.nullptr) return; final size = calloc<_Coord>() - ..ref.x = cols - ..ref.y = rows; + ..ref.x = clampPtyDimension(cols) + ..ref.y = clampPtyDimension(rows); _resizePseudoConsole(_hpc, size.ref); calloc.free(size); } @@ -587,7 +588,12 @@ class WindowsPty implements PtySession { /// Resolve a bare command name against the environment's PATH + /// PATHEXT (mirrors what the POSIX side does with `:`-split PATH — /// visible/debuggable resolution instead of CreateProcess magic). - static String _resolveExecutable(String executable, Map environment) { + /// + /// [exists] overrides the on-disk probe so the resolution logic is + /// unit-testable off-Windows; production passes the default. Public for + /// that reason — not part of the backend's external contract. + static String resolveExecutable(String executable, Map environment, {bool Function(String path)? exists}) { + final fileExists = exists ?? ((String path) => File(path).existsSync()); final pathext = (environment['PATHEXT'] ?? Platform.environment['PATHEXT'] ?? '.COM;.EXE;.BAT;.CMD').split(';').where((e) => e.isNotEmpty).toList(); final hasKnownExt = pathext.any((e) => executable.toLowerCase().endsWith(e.toLowerCase())); @@ -604,7 +610,7 @@ class WindowsPty implements PtySession { if (executable.contains('\\') || executable.contains('/')) { for (final c in candidates(executable)) { - if (File(c).existsSync()) return c; + if (fileExists(c)) return c; } return executable; } @@ -612,14 +618,15 @@ class WindowsPty implements PtySession { for (final dir in path.split(';')) { if (dir.isEmpty) continue; for (final c in candidates('$dir\\$executable')) { - if (File(c).existsSync()) return c; + if (fileExists(c)) return c; } } return executable; } - /// Quote one argument per MSVCRT command-line parsing rules. - static String _quoteArg(String arg) { + /// Quote one argument per MSVCRT command-line parsing rules. Public so the + /// quoting rules can be unit-tested off-Windows; not an external contract. + static String quoteArg(String arg) { if (arg.isNotEmpty && !arg.contains(RegExp(r'[ \t"\n\v]'))) return arg; final b = StringBuffer('"'); var backslashes = 0; @@ -646,17 +653,17 @@ class WindowsPty implements PtySession { return b.toString(); } - /// Compose a CREATE_UNICODE_ENVIRONMENT block: `K=V\0...\0\0`, - /// entries sorted case-insensitively by key per CreateProcess docs. - static ffi.Pointer _environmentBlock(Map environment) { + /// Compose the body of a CREATE_UNICODE_ENVIRONMENT block: `K=V\0...\0` + /// with one trailing NUL, entries sorted case-insensitively by key per + /// CreateProcess docs. The caller nativizes via `toNativeUtf16`, whose own + /// terminator completes the required double-NUL ending (which also keeps an + /// empty environment block valid). Public for unit testing. + static String composeEnvironmentBlock(Map environment) { final entries = environment.entries.toList()..sort((a, b) => a.key.toUpperCase().compareTo(b.key.toUpperCase())); // NUL via fromCharCode — an inline NUL escape in a string literal // is invisible in review and trips up text tooling. final nul = String.fromCharCode(0); final joined = entries.map((e) => '${e.key}=${e.value}$nul').join(); - // toNativeUtf16 appends the final terminating NUL; the explicit one - // after the last entry completes the required double-NUL ending (and - // keeps an empty environment block valid too). - return '$joined$nul'.toNativeUtf16(allocator: malloc); + return '$joined$nul'; } } diff --git a/test/pty/pty_size_test.dart b/test/pty/pty_size_test.dart new file mode 100644 index 00000000..46a88847 --- /dev/null +++ b/test/pty/pty_size_test.dart @@ -0,0 +1,23 @@ +/// Unit tests for the shared PTY-dimension clamp (`pty_size.dart`). Both +/// backends route spawn + resize through it so a degenerate (0/1) terminal +/// size can never reach a child — see microsoft/terminal#19922. +library; + +import 'package:clide/src/pty/pty_size.dart'; +import 'package:test/test.dart'; + +void main() { + group('clampPtyDimension', () { + test('raises sub-minimum values to the floor', () { + expect(clampPtyDimension(0), minPtyDimension); + expect(clampPtyDimension(1), minPtyDimension); + expect(clampPtyDimension(-5), minPtyDimension); + }); + + test('passes through values at or above the floor', () { + expect(clampPtyDimension(minPtyDimension), minPtyDimension); + expect(clampPtyDimension(80), 80); + expect(clampPtyDimension(24), 24); + }); + }); +} diff --git a/test/pty/windows_pty_args_test.dart b/test/pty/windows_pty_args_test.dart new file mode 100644 index 00000000..273c9f7c --- /dev/null +++ b/test/pty/windows_pty_args_test.dart @@ -0,0 +1,93 @@ +/// Cross-platform unit tests for the pure Windows-backend helpers in +/// `windows_pty.dart`: MSVCRT command-line quoting, PATH/PATHEXT executable +/// resolution, and CreateProcess environment-block composition. +/// +/// These touch no Win32 API, so they run on every platform — the FFI +/// bindings in windows_pty.dart are lazily initialized top-level finals and +/// are never accessed here. This is the off-Windows coverage for logic the +/// `windows_pty_test.dart` smoke suite can only exercise on Windows. +library; + +import 'package:clide/src/pty/windows_pty.dart'; +import 'package:test/test.dart'; + +void main() { + group('quoteArg (MSVCRT command-line rules)', () { + test('leaves an argument with no special chars untouched', () { + expect(WindowsPty.quoteArg('simple'), 'simple'); + expect(WindowsPty.quoteArg('C:\\path\\to\\tool.exe'), 'C:\\path\\to\\tool.exe'); + }); + + test('quotes an empty argument so it survives as a distinct token', () { + expect(WindowsPty.quoteArg(''), '""'); + }); + + test('quotes arguments containing spaces or tabs', () { + expect(WindowsPty.quoteArg('has space'), '"has space"'); + expect(WindowsPty.quoteArg('has\ttab'), '"has\ttab"'); + }); + + test('escapes an embedded double quote with a backslash', () { + // a"b -> "a\"b" + expect(WindowsPty.quoteArg('a"b'), '"a\\"b"'); + }); + + test('doubles a run of backslashes that precedes the closing quote', () { + // a b\ -> "a b\\" (trailing backslash doubled before the ") + expect(WindowsPty.quoteArg('a b\\'), '"a b\\\\"'); + }); + + test('backslashes before an embedded quote are doubled, plus one to escape it', () { + // a\"b -> "a\\\"b" + expect(WindowsPty.quoteArg('a\\"b'), '"a\\\\\\"b"'); + }); + }); + + group('composeEnvironmentBlock', () { + final z = String.fromCharCode(0); + + test('sorts entries case-insensitively, NUL-terminates each, ends double-NUL', () { + final block = WindowsPty.composeEnvironmentBlock({'bee': '2', 'Apple': '1', 'cat': '3'}); + expect(block, 'Apple=1${z}bee=2${z}cat=3$z$z'); + }); + + test('an empty environment is a single NUL (toNativeUtf16 adds the second)', () { + expect(WindowsPty.composeEnvironmentBlock({}), z); + }); + + test('preserves = and values verbatim', () { + expect(WindowsPty.composeEnvironmentBlock({'PATH': r'C:\a;C:\b'}), 'PATH=C:\\a;C:\\b$z$z'); + }); + }); + + group('resolveExecutable (PATH + PATHEXT, injected existence probe)', () { + test('returns a path with a known extension as-is when it exists', () { + final r = WindowsPty.resolveExecutable('C:\\tools\\foo.exe', {'PATHEXT': '.EXE'}, exists: (p) => p == 'C:\\tools\\foo.exe'); + expect(r, 'C:\\tools\\foo.exe'); + }); + + test('appends a PATHEXT extension to a bare name found on PATH', () { + final r = WindowsPty.resolveExecutable('foo', {'PATH': 'C:\\bin;C:\\other', 'PATHEXT': '.COM;.EXE'}, exists: (p) => p == 'C:\\bin\\foo.EXE'); + expect(r, 'C:\\bin\\foo.EXE'); + }); + + test('tries PATH dirs in order and stops at the first hit', () { + final probed = []; + final r = WindowsPty.resolveExecutable( + 'bar', + {'PATH': 'C:\\a;C:\\b', 'PATHEXT': '.EXE'}, + exists: (p) { + probed.add(p); + return p == 'C:\\b\\bar.EXE'; + }, + ); + expect(r, 'C:\\b\\bar.EXE'); + expect(probed, contains('C:\\a\\bar')); // probed the first dir before the hit in the second + }); + + test('returns the bare name unchanged when nothing resolves', () { + final r = WindowsPty.resolveExecutable('nope', {'PATH': 'C:\\bin', 'PATHEXT': '.EXE'}, exists: (_) => false); + expect(r, 'nope'); + }); + }); +}