From 1827764be3ab1ea8b96666ad58e33365aa9b372b Mon Sep 17 00:00:00 2001 From: Marcos Sevilla Date: Thu, 1 Oct 2026 15:07:16 +0200 Subject: [PATCH 1/5] refactor(mcp): reduce cognitive complexity of the test tool Co-Authored-By: Claude Opus 5.5 (1M context) --- lib/src/mcp/mcp_server.dart | 287 +++++++++++++++++++----------------- 1 file changed, 149 insertions(+), 138 deletions(-) diff --git a/lib/src/mcp/mcp_server.dart b/lib/src/mcp/mcp_server.dart index ed753ea26..57dd710ab 100644 --- a/lib/src/mcp/mcp_server.dart +++ b/lib/src/mcp/mcp_server.dart @@ -402,90 +402,66 @@ Only one value can be selected. } List _parseTest(Map args) { + final timeoutSeconds = args['timeout_seconds'] as num?; + final paths = args['paths'] as List? ?? const []; + // NOTE: 'directory' is intentionally not added here. It is applied as the // working directory in [_runToolCommand], not as a positional test target. - final cliArgs = [if (args['dart'] == true) 'dart', 'test']; - - if (args['coverage'] == true) { - cliArgs.add('--coverage'); - } - if (args['recursive'] == true) { - cliArgs.add('-r'); - } - if (args['optimization'] == false) { - cliArgs.add('--no-optimization'); - } - if (args['concurrency'] != null) { - cliArgs.addAll(['-j', args['concurrency']! as String]); - } - if (args['tags'] != null) { - cliArgs.addAll(['-t', args['tags']! as String]); - } - if (args['exclude_coverage'] != null) { - cliArgs.addAll([ - '--exclude-coverage', - args['exclude_coverage']! as String, - ]); - } - if (args['exclude_tags'] != null) { - cliArgs.addAll(['-x', args['exclude_tags']! as String]); - } - if (args['min_coverage'] != null) { - cliArgs.addAll(['--min-coverage', args['min_coverage']! as String]); - } - if (args['test_randomize_ordering_seed'] != null) { - cliArgs.addAll([ + return [ + ..._flag(args, 'dart', 'dart'), + 'test', + ..._flag(args, 'coverage', '--coverage'), + ..._flag(args, 'recursive', '-r'), + ..._flag(args, 'optimization', '--no-optimization', whenValue: false), + ..._option(args, 'concurrency', '-j'), + ..._option(args, 'tags', '-t'), + ..._option(args, 'exclude_coverage', '--exclude-coverage'), + ..._option(args, 'exclude_tags', '-x'), + ..._option(args, 'min_coverage', '--min-coverage'), + ..._option( + args, + 'test_randomize_ordering_seed', '--test-randomize-ordering-seed', - args['test_randomize_ordering_seed']! as String, - ]); - } - if (args['update_goldens'] == true) { - cliArgs.add('--update-goldens'); - } - if (args['force_ansi'] == true) { - cliArgs.add('--force-ansi'); - } - if (args['dart-define'] != null) { - cliArgs.addAll(['--dart-define', args['dart-define']! as String]); - } - if (args['dart-define-from-file'] != null) { - cliArgs.addAll([ - '--dart-define-from-file', - args['dart-define-from-file']! as String, - ]); - } - if (args['platform'] != null) { - cliArgs.addAll(['--platform', args['platform']! as String]); - } - if (args['run_skipped'] == true) { - cliArgs.add('--run-skipped'); - } - if (args['check_ignore'] == true) { - cliArgs.add('--check-ignore'); - } - if (args['show_uncovered'] == true) { - cliArgs.add('--show-uncovered'); - } - if (args['timeout_seconds'] != null) { - cliArgs.addAll([ + ), + ..._flag(args, 'update_goldens', '--update-goldens'), + ..._flag(args, 'force_ansi', '--force-ansi'), + ..._option(args, 'dart-define', '--dart-define'), + ..._option(args, 'dart-define-from-file', '--dart-define-from-file'), + ..._option(args, 'platform', '--platform'), + ..._flag(args, 'run_skipped', '--run-skipped'), + ..._flag(args, 'check_ignore', '--check-ignore'), + ..._flag(args, 'show_uncovered', '--show-uncovered'), + if (timeoutSeconds != null) ...[ '--timeout', - (args['timeout_seconds']! as num).toInt().toString(), - ]); - } - - // Positional test targets go last, after every option, so that they are - // parsed as `rest` rather than as a value for the preceding option. The - // `--` terminator keeps a target that begins with `-` from being read as - // an option; the parser strips it back out of `rest`, so the test command - // sees the paths and nothing else. - final paths = args['paths'] as List?; - if (paths != null && paths.isNotEmpty) { - cliArgs.addAll(['--', ...paths.cast()]); - } - - return cliArgs; + timeoutSeconds.toInt().toString(), + ], + // Positional test targets go last, after every option, so that they are + // parsed as `rest` rather than as a value for the preceding option. The + // `--` terminator keeps a target that begins with `-` from being read as + // an option; the parser strips it back out of `rest`, so the test command + // sees the paths and nothing else. + if (paths.isNotEmpty) ...['--', ...paths.cast()], + ]; } + /// Returns `[flag]` when `args[key]` equals [whenValue], else nothing. + static List _flag( + Map args, + String key, + String flag, { + bool whenValue = true, + }) => args[key] == whenValue ? [flag] : const []; + + /// Returns `[option, value]` when `args[key]` is set, else nothing. + static List _option( + Map args, + String key, + String option, + ) => switch (args[key] as String?) { + final value? => [option, value], + null => const [], + }; + List _parsePackagesGet(Map args) { // NOTE: 'directory' is applied as the working directory in // [_runToolCommand], not as a positional argument. @@ -591,47 +567,12 @@ Only one value can be selected. Map? requestArguments, }) { return _lock.run(() async { - final commandString = 'very_good ${args.join(' ')}'; - final output = StringBuffer(); - - Future runCaptured(Future Function(Logger logger) body) { - final sink = CapturingStdout(output); - return IOOverrides.runZoned( - () => body(Logger()), - stdout: () => sink, - stderr: () => sink, - ); - } - - // Builds a structured JSON failure result from [reason] and - // [failureType]. The captured command output is surfaced as - // `partialResults` so any diagnostics emitted before a failure or throw - // are preserved. A short human-readable summary is also logged to the - // real stderr (the stdio transport forbids non-JSON on stdout, so stderr - // is free for diagnostics). - CallToolResult errorResult( - String reason, { - required ToolFailureType failureType, - StackTrace? stackTrace, - }) { - final captured = sanitizeCommandOutput(output.toString()).trim(); - stderr.writeln( - '[very_good_mcp] "$toolName" ${failureType.name} error: $reason ' - '(command: $commandString)', - ); - if (stackTrace != null) { - stderr.writeln('[very_good_mcp] Stack trace: $stackTrace'); - } - return StructuredToolError( - toolName: toolName, - reason: reason, - failureType: failureType, - commandString: commandString, - directory: directory, - attemptedArguments: requestArguments, - capturedOutput: captured, - ).toCallToolResult(); - } + final run = _ToolRun( + toolName: toolName, + commandString: 'very_good ${args.join(' ')}', + directory: directory, + requestArguments: requestArguments, + ); // Apply [directory] as the real working directory for the duration of // the run, restoring it afterwards. The underlying commands resolve their @@ -642,32 +583,17 @@ Only one value can be selected. try { if (directory != null) Directory.current = directory; - final exitCode = await runCaptured( + final exitCode = await run.capture( (logger) => _commandRunnerBuilder(logger: logger).run(args), ); - - if (exitCode == ExitCode.success.code) { - final captured = sanitizeCommandOutput(output.toString()).trim(); - return CallToolResult( - content: [ - TextContent(text: '"$toolName" completed successfully.'), - if (captured.isNotEmpty) TextContent(text: captured), - ], - isError: false, - ); - } - - return errorResult( - 'failed with exit code $exitCode.', - failureType: ToolFailureType.fromExitCode(exitCode), - ); + return run.resultFor(exitCode); } on UsageException catch (e) { - return errorResult( + return run.failure( 'usage error: ${e.message}', failureType: ToolFailureType.validation, ); } on Exception catch (e, stackTrace) { - return errorResult( + return run.failure( 'threw an exception: $e', failureType: ToolFailureType.transient, stackTrace: stackTrace, @@ -679,6 +605,91 @@ Only one value can be selected. } } +/// A single in-process tool invocation run by +/// [VeryGoodMCPServer._runToolCommand]. +/// +/// Owns the buffer that captures the command's output and turns the outcome +/// of the run into a [CallToolResult]. +class _ToolRun { + new({ + required this.toolName, + required this.commandString, + required this.directory, + required this.requestArguments, + }); + + final String toolName; + final String commandString; + final String? directory; + final Map? requestArguments; + final StringBuffer _output = StringBuffer(); + + String get _capturedOutput => + sanitizeCommandOutput(_output.toString()).trim(); + + /// Runs [body] with `stdout`/`stderr` redirected into this run's buffer. + /// + /// The [Logger] is built inside the zone because mason captures + /// `IOOverrides.current` at construction time. + Future capture(Future Function(Logger logger) body) { + final sink = CapturingStdout(_output); + return IOOverrides.runZoned( + () => body(Logger()), + stdout: () => sink, + stderr: () => sink, + ); + } + + /// Maps the command's [exitCode] to a success or failure result. + CallToolResult resultFor(int exitCode) => exitCode == ExitCode.success.code + ? _success() + : failure( + 'failed with exit code $exitCode.', + failureType: ToolFailureType.fromExitCode(exitCode), + ); + + CallToolResult _success() { + final captured = _capturedOutput; + return CallToolResult( + content: [ + TextContent(text: '"$toolName" completed successfully.'), + if (captured.isNotEmpty) TextContent(text: captured), + ], + isError: false, + ); + } + + /// Builds a structured JSON failure result from [reason] and [failureType]. + /// + /// The captured command output is surfaced as `partialResults` so any + /// diagnostics emitted before a failure or throw are preserved. A short + /// human-readable summary is also logged to the real stderr (the stdio + /// transport forbids non-JSON on stdout, so stderr is free for diagnostics). + CallToolResult failure( + String reason, { + required ToolFailureType failureType, + StackTrace? stackTrace, + }) { + final captured = _capturedOutput; + stderr.writeln( + '[very_good_mcp] "$toolName" ${failureType.name} error: $reason ' + '(command: $commandString)', + ); + if (stackTrace != null) { + stderr.writeln('[very_good_mcp] Stack trace: $stackTrace'); + } + return StructuredToolError( + toolName: toolName, + reason: reason, + failureType: failureType, + commandString: commandString, + directory: directory, + attemptedArguments: requestArguments, + capturedOutput: captured, + ).toCallToolResult(); + } +} + /// A [Stdout] that captures everything written to it into a [StringBuffer] /// instead of the real process stdout/stderr. /// From ff2761cdfb65210f145bc50bb5349585356af633 Mon Sep 17 00:00:00 2001 From: Marcos Sevilla Date: Mon, 5 Oct 2026 18:49:03 +0300 Subject: [PATCH 2/5] refactor(mcp): move ToolRun to its own file Address review feedback: ToolRun now lives in tool_run.dart together with the CapturingStdout and sanitizeCommandOutput helpers it owns, avoiding a circular import with mcp_server.dart. ToolRun.capture now returns the command's exit code directly instead of being generic. --- lib/src/mcp/mcp_server.dart | 197 +--------------------------- lib/src/mcp/tool_run.dart | 209 ++++++++++++++++++++++++++++++ test/src/mcp/mcp_server_test.dart | 1 + 3 files changed, 213 insertions(+), 194 deletions(-) create mode 100644 lib/src/mcp/tool_run.dart diff --git a/lib/src/mcp/mcp_server.dart b/lib/src/mcp/mcp_server.dart index 57dd710ab..1c23d715c 100644 --- a/lib/src/mcp/mcp_server.dart +++ b/lib/src/mcp/mcp_server.dart @@ -1,7 +1,5 @@ import 'dart:async'; -import 'dart:convert'; -import 'dart:io' - show Directory, IOOverrides, IOSink, Stdout, StdoutException, stderr; +import 'dart:io' show Directory, IOOverrides; import 'package:args/command_runner.dart'; import 'package:dart_mcp/server.dart'; @@ -12,6 +10,7 @@ import 'package:very_good_cli/src/command_runner.dart'; import 'package:very_good_cli/src/coverage/coverage.dart'; import 'package:very_good_cli/src/mcp/lock.dart'; import 'package:very_good_cli/src/mcp/structured_tool_error.dart'; +import 'package:very_good_cli/src/mcp/tool_run.dart'; import 'package:very_good_cli/src/version.dart'; /// {@template command_runner_builder} @@ -567,7 +566,7 @@ Only one value can be selected. Map? requestArguments, }) { return _lock.run(() async { - final run = _ToolRun( + final run = ToolRun( toolName: toolName, commandString: 'very_good ${args.join(' ')}', directory: directory, @@ -604,193 +603,3 @@ Only one value can be selected. }); } } - -/// A single in-process tool invocation run by -/// [VeryGoodMCPServer._runToolCommand]. -/// -/// Owns the buffer that captures the command's output and turns the outcome -/// of the run into a [CallToolResult]. -class _ToolRun { - new({ - required this.toolName, - required this.commandString, - required this.directory, - required this.requestArguments, - }); - - final String toolName; - final String commandString; - final String? directory; - final Map? requestArguments; - final StringBuffer _output = StringBuffer(); - - String get _capturedOutput => - sanitizeCommandOutput(_output.toString()).trim(); - - /// Runs [body] with `stdout`/`stderr` redirected into this run's buffer. - /// - /// The [Logger] is built inside the zone because mason captures - /// `IOOverrides.current` at construction time. - Future capture(Future Function(Logger logger) body) { - final sink = CapturingStdout(_output); - return IOOverrides.runZoned( - () => body(Logger()), - stdout: () => sink, - stderr: () => sink, - ); - } - - /// Maps the command's [exitCode] to a success or failure result. - CallToolResult resultFor(int exitCode) => exitCode == ExitCode.success.code - ? _success() - : failure( - 'failed with exit code $exitCode.', - failureType: ToolFailureType.fromExitCode(exitCode), - ); - - CallToolResult _success() { - final captured = _capturedOutput; - return CallToolResult( - content: [ - TextContent(text: '"$toolName" completed successfully.'), - if (captured.isNotEmpty) TextContent(text: captured), - ], - isError: false, - ); - } - - /// Builds a structured JSON failure result from [reason] and [failureType]. - /// - /// The captured command output is surfaced as `partialResults` so any - /// diagnostics emitted before a failure or throw are preserved. A short - /// human-readable summary is also logged to the real stderr (the stdio - /// transport forbids non-JSON on stdout, so stderr is free for diagnostics). - CallToolResult failure( - String reason, { - required ToolFailureType failureType, - StackTrace? stackTrace, - }) { - final captured = _capturedOutput; - stderr.writeln( - '[very_good_mcp] "$toolName" ${failureType.name} error: $reason ' - '(command: $commandString)', - ); - if (stackTrace != null) { - stderr.writeln('[very_good_mcp] Stack trace: $stackTrace'); - } - return StructuredToolError( - toolName: toolName, - reason: reason, - failureType: failureType, - commandString: commandString, - directory: directory, - attemptedArguments: requestArguments, - capturedOutput: captured, - ).toCallToolResult(); - } -} - -/// A [Stdout] that captures everything written to it into a [StringBuffer] -/// instead of the real process stdout/stderr. -/// -/// Used to redirect a command's in-process [Logger] output (which mason routes -/// through `stdout`/`stderr`, including progress spinners) away from the real -/// stdout shared with the MCP JSON-RPC stream. It reports no terminal so mason -/// emits plain, animation-free lines. -@visibleForTesting -class CapturingStdout implements Stdout { - /// Creates a [CapturingStdout] that appends all writes to [_buffer]. - new(this._buffer); - - final StringBuffer _buffer; - - @override - Encoding encoding = utf8; - - @override - String lineTerminator = '\n'; - - @override - void write(Object? object) => _buffer.write(object ?? 'null'); - - @override - void writeln([Object? object = '']) => _buffer.writeln(object ?? ''); - - @override - void writeAll(Iterable objects, [String separator = '']) => - _buffer.writeAll(objects, separator); - - @override - void writeCharCode(int charCode) => _buffer.writeCharCode(charCode); - - @override - void add(List data) { - try { - _buffer.write(encoding.decode(data)); - } on FormatException { - _buffer.write(String.fromCharCodes(data)); - } - } - - @override - void addError(Object error, [StackTrace? stackTrace]) {} - - @override - Future addStream(Stream> stream) => stream.forEach(add); - - @override - Future flush() async {} - - @override - Future close() async {} - - @override - Future get done => Future.value(); - - @override - bool get hasTerminal => false; - - @override - bool get supportsAnsiEscapes => false; - - @override - int get terminalColumns { - throw const StdoutException('No terminal attached'); - } - - @override - int get terminalLines { - throw const StdoutException('No terminal attached'); - } - - @override - IOSink get nonBlocking => this; -} - -/// Matches a CSI ANSI escape sequence (colors, cursor moves, line erases). -final _ansiEscape = RegExp(r'\x1B\[[0-?]*[ -/]*[@-~]'); - -/// Renders raw captured command output as plain text for a tool result. -/// -/// In-process commands (and the test subprocesses they reformat) animate -/// progress with ANSI escape sequences and carriage returns: a spinner redraws -/// a single line in place with `\r` and erases it with `\x1B[2K`. A terminal -/// resolves those to clean lines, but the raw bytes surfaced to an MCP client -/// collapse into one run-on line. This reproduces the terminal's settled view: -/// -/// * strips ANSI escape sequences; -/// * normalizes `\r\n` to `\n`; and -/// * collapses carriage-return redraws to the text after the last `\r` on each -/// line (the final state the user would see), trimming trailing padding. -@visibleForTesting -String sanitizeCommandOutput(String raw) { - return raw - .replaceAll(_ansiEscape, '') - .replaceAll('\r\n', '\n') - .split('\n') - .map((line) { - final output = line.contains('\r') ? line.split('\r').last : line; - return output.trimRight(); - }) - .join('\n'); -} diff --git a/lib/src/mcp/tool_run.dart b/lib/src/mcp/tool_run.dart new file mode 100644 index 000000000..817a5b1bc --- /dev/null +++ b/lib/src/mcp/tool_run.dart @@ -0,0 +1,209 @@ +import 'dart:async'; +import 'dart:convert'; +import 'dart:io' show IOOverrides, IOSink, Stdout, StdoutException, stderr; + +import 'package:dart_mcp/server.dart'; +import 'package:mason/mason.dart' hide packageVersion; +import 'package:meta/meta.dart'; +import 'package:very_good_cli/src/mcp/structured_tool_error.dart'; + +/// {@template tool_run} +/// A single in-process tool invocation run by the MCP server. +/// +/// Owns the buffer that captures the command's output and turns the outcome +/// of the run into a [CallToolResult]. +/// {@endtemplate} +class ToolRun { + /// {@macro tool_run} + new({ + required this.toolName, + required this.commandString, + required this.directory, + required this.requestArguments, + }); + + /// The name of the MCP tool being run. + final String toolName; + + /// The equivalent `very_good` command line, used for diagnostics. + final String commandString; + + /// The working directory the command runs in, if any. + final String? directory; + + /// The raw arguments of the MCP tool request, if any. + final Map? requestArguments; + + final StringBuffer _output = StringBuffer(); + + String get _capturedOutput => + sanitizeCommandOutput(_output.toString()).trim(); + + /// Runs [command] with `stdout`/`stderr` redirected into this run's buffer + /// and returns its exit code. + /// + /// The [Logger] is built inside the zone because mason captures + /// `IOOverrides.current` at construction time. + Future capture(Future Function(Logger logger) command) { + final sink = CapturingStdout(_output); + return IOOverrides.runZoned( + () => command(Logger()), + stdout: () => sink, + stderr: () => sink, + ); + } + + /// Maps the command's [exitCode] to a success or failure result. + CallToolResult resultFor(int exitCode) => exitCode == ExitCode.success.code + ? _success() + : failure( + 'failed with exit code $exitCode.', + failureType: ToolFailureType.fromExitCode(exitCode), + ); + + CallToolResult _success() { + final captured = _capturedOutput; + return CallToolResult( + content: [ + TextContent(text: '"$toolName" completed successfully.'), + if (captured.isNotEmpty) TextContent(text: captured), + ], + isError: false, + ); + } + + /// Builds a structured JSON failure result from [reason] and [failureType]. + /// + /// The captured command output is surfaced as `partialResults` so any + /// diagnostics emitted before a failure or throw are preserved. A short + /// human-readable summary is also logged to the real stderr (the stdio + /// transport forbids non-JSON on stdout, so stderr is free for diagnostics). + CallToolResult failure( + String reason, { + required ToolFailureType failureType, + StackTrace? stackTrace, + }) { + final captured = _capturedOutput; + stderr.writeln( + '[very_good_mcp] "$toolName" ${failureType.name} error: $reason ' + '(command: $commandString)', + ); + if (stackTrace != null) { + stderr.writeln('[very_good_mcp] Stack trace: $stackTrace'); + } + return StructuredToolError( + toolName: toolName, + reason: reason, + failureType: failureType, + commandString: commandString, + directory: directory, + attemptedArguments: requestArguments, + capturedOutput: captured, + ).toCallToolResult(); + } +} + +/// A [Stdout] that captures everything written to it into a [StringBuffer] +/// instead of the real process stdout/stderr. +/// +/// Used to redirect a command's in-process [Logger] output (which mason routes +/// through `stdout`/`stderr`, including progress spinners) away from the real +/// stdout shared with the MCP JSON-RPC stream. It reports no terminal so mason +/// emits plain, animation-free lines. +@visibleForTesting +class CapturingStdout implements Stdout { + /// Creates a [CapturingStdout] that appends all writes to [_buffer]. + new(this._buffer); + + final StringBuffer _buffer; + + @override + Encoding encoding = utf8; + + @override + String lineTerminator = '\n'; + + @override + void write(Object? object) => _buffer.write(object ?? 'null'); + + @override + void writeln([Object? object = '']) => _buffer.writeln(object ?? ''); + + @override + void writeAll(Iterable objects, [String separator = '']) => + _buffer.writeAll(objects, separator); + + @override + void writeCharCode(int charCode) => _buffer.writeCharCode(charCode); + + @override + void add(List data) { + try { + _buffer.write(encoding.decode(data)); + } on FormatException { + _buffer.write(String.fromCharCodes(data)); + } + } + + @override + void addError(Object error, [StackTrace? stackTrace]) {} + + @override + Future addStream(Stream> stream) => stream.forEach(add); + + @override + Future flush() async {} + + @override + Future close() async {} + + @override + Future get done => Future.value(); + + @override + bool get hasTerminal => false; + + @override + bool get supportsAnsiEscapes => false; + + @override + int get terminalColumns { + throw const StdoutException('No terminal attached'); + } + + @override + int get terminalLines { + throw const StdoutException('No terminal attached'); + } + + @override + IOSink get nonBlocking => this; +} + +/// Matches a CSI ANSI escape sequence (colors, cursor moves, line erases). +final _ansiEscape = RegExp(r'\x1B\[[0-?]*[ -/]*[@-~]'); + +/// Renders raw captured command output as plain text for a tool result. +/// +/// In-process commands (and the test subprocesses they reformat) animate +/// progress with ANSI escape sequences and carriage returns: a spinner redraws +/// a single line in place with `\r` and erases it with `\x1B[2K`. A terminal +/// resolves those to clean lines, but the raw bytes surfaced to an MCP client +/// collapse into one run-on line. This reproduces the terminal's settled view: +/// +/// * strips ANSI escape sequences; +/// * normalizes `\r\n` to `\n`; and +/// * collapses carriage-return redraws to the text after the last `\r` on each +/// line (the final state the user would see), trimming trailing padding. +@visibleForTesting +String sanitizeCommandOutput(String raw) { + return raw + .replaceAll(_ansiEscape, '') + .replaceAll('\r\n', '\n') + .split('\n') + .map((line) { + final output = line.contains('\r') ? line.split('\r').last : line; + return output.trimRight(); + }) + .join('\n'); +} diff --git a/test/src/mcp/mcp_server_test.dart b/test/src/mcp/mcp_server_test.dart index 1a9fa0ed8..6ae8f6b43 100644 --- a/test/src/mcp/mcp_server_test.dart +++ b/test/src/mcp/mcp_server_test.dart @@ -10,6 +10,7 @@ import 'package:stream_channel/stream_channel.dart'; import 'package:test/test.dart'; import 'package:very_good_cli/src/command_runner.dart'; import 'package:very_good_cli/src/mcp/mcp_server.dart'; +import 'package:very_good_cli/src/mcp/tool_run.dart'; /// Decodes a MCP tool result's text content as the structured error payload. Map _errorPayload(CallToolResult result) { From 0870f9e279d0b3ad9ea5d4f66376a1c747c68ec7 Mon Sep 17 00:00:00 2001 From: Marcos Sevilla Date: Mon, 5 Oct 2026 18:52:45 +0300 Subject: [PATCH 3/5] test(mcp): move ToolRun helper tests to tool_run_test.dart CapturingStdout and sanitizeCommandOutput now live in tool_run.dart, so their tests move to the matching test file. --- test/src/mcp/mcp_server_test.dart | 91 ----------------------------- test/src/mcp/tool_run_test.dart | 97 +++++++++++++++++++++++++++++++ 2 files changed, 97 insertions(+), 91 deletions(-) create mode 100644 test/src/mcp/tool_run_test.dart diff --git a/test/src/mcp/mcp_server_test.dart b/test/src/mcp/mcp_server_test.dart index 6ae8f6b43..832827238 100644 --- a/test/src/mcp/mcp_server_test.dart +++ b/test/src/mcp/mcp_server_test.dart @@ -10,7 +10,6 @@ import 'package:stream_channel/stream_channel.dart'; import 'package:test/test.dart'; import 'package:very_good_cli/src/command_runner.dart'; import 'package:very_good_cli/src/mcp/mcp_server.dart'; -import 'package:very_good_cli/src/mcp/tool_run.dart'; /// Decodes a MCP tool result's text content as the structured error payload. Map _errorPayload(CallToolResult result) { @@ -1033,94 +1032,4 @@ void main() { ); }); }); - - group('CapturingStdout', () { - late StringBuffer buffer; - late CapturingStdout capturing; - - setUp(() { - buffer = StringBuffer(); - capturing = CapturingStdout(buffer); - }); - - test('write/writeln/writeAll/writeCharCode append to the buffer', () { - capturing - ..write('a') - ..write(null) - ..writeln('b') - ..writeln() - ..writeAll(['c', 'd'], '-') - ..writeCharCode(0x65); // 'e' - expect(buffer.toString(), equals('anullb\n\nc-de')); - }); - - test('add decodes bytes with the current encoding', () { - capturing.add(utf8.encode('héllo')); - expect(buffer.toString(), equals('héllo')); - }); - - test('add falls back to char codes on malformed bytes', () { - capturing.add([0xff, 0xfe]); - expect(buffer.toString(), equals(String.fromCharCodes([0xff, 0xfe]))); - }); - - test('addStream forwards all chunks', () async { - await capturing.addStream( - Stream.fromIterable([utf8.encode('x'), utf8.encode('y')]), - ); - expect(buffer.toString(), equals('xy')); - }); - - test('reports no terminal and tolerates sink lifecycle calls', () async { - expect(capturing.hasTerminal, isFalse); - expect(capturing.supportsAnsiEscapes, isFalse); - expect(capturing.nonBlocking, same(capturing)); - expect(capturing.encoding, equals(utf8)); - expect(capturing.lineTerminator, equals('\n')); - expect(() => capturing.terminalColumns, throwsA(isA())); - expect(() => capturing.terminalLines, throwsA(isA())); - capturing.addError('ignored'); - await expectLater(capturing.flush(), completes); - await expectLater(capturing.close(), completes); - await expectLater(capturing.done, completes); - }); - }); - - group('sanitizeCommandOutput', () { - test('strips ANSI escape sequences', () { - expect( - sanitizeCommandOutput('\x1B[31mred\x1B[0m and \x1B[1mbold\x1B[0m'), - equals('red and bold'), - ); - }); - - test('normalizes CRLF to LF', () { - expect(sanitizeCommandOutput('a\r\nb\r\nc'), equals('a\nb\nc')); - }); - - test('collapses carriage-return redraws to the settled text', () { - // A spinner redrawing one line in place: erase + rewrite per tick. - expect( - sanitizeCommandOutput( - '\r\x1B[2K00:01 +1\r\x1B[2K00:02 +5\r\x1B[2KAll tests passed!', - ), - equals('All tests passed!'), - ); - }); - - test('preserves genuine newlines while collapsing redraws per line', () { - expect( - sanitizeCommandOutput('compiling...\rdone\nAll tests passed!'), - equals('done\nAll tests passed!'), - ); - }); - - test('trims trailing padding left by line erases', () { - expect(sanitizeCommandOutput('result '), equals('result')); - }); - - test('leaves plain multi-line output untouched', () { - expect(sanitizeCommandOutput('line 1\nline 2'), equals('line 1\nline 2')); - }); - }); } diff --git a/test/src/mcp/tool_run_test.dart b/test/src/mcp/tool_run_test.dart new file mode 100644 index 000000000..5b106362a --- /dev/null +++ b/test/src/mcp/tool_run_test.dart @@ -0,0 +1,97 @@ +import 'dart:convert'; +import 'dart:io'; + +import 'package:test/test.dart'; +import 'package:very_good_cli/src/mcp/tool_run.dart'; + +void main() { + group('CapturingStdout', () { + late StringBuffer buffer; + late CapturingStdout capturing; + + setUp(() { + buffer = StringBuffer(); + capturing = CapturingStdout(buffer); + }); + + test('write/writeln/writeAll/writeCharCode append to the buffer', () { + capturing + ..write('a') + ..write(null) + ..writeln('b') + ..writeln() + ..writeAll(['c', 'd'], '-') + ..writeCharCode(0x65); // 'e' + expect(buffer.toString(), equals('anullb\n\nc-de')); + }); + + test('add decodes bytes with the current encoding', () { + capturing.add(utf8.encode('héllo')); + expect(buffer.toString(), equals('héllo')); + }); + + test('add falls back to char codes on malformed bytes', () { + capturing.add([0xff, 0xfe]); + expect(buffer.toString(), equals(String.fromCharCodes([0xff, 0xfe]))); + }); + + test('addStream forwards all chunks', () async { + await capturing.addStream( + Stream.fromIterable([utf8.encode('x'), utf8.encode('y')]), + ); + expect(buffer.toString(), equals('xy')); + }); + + test('reports no terminal and tolerates sink lifecycle calls', () async { + expect(capturing.hasTerminal, isFalse); + expect(capturing.supportsAnsiEscapes, isFalse); + expect(capturing.nonBlocking, same(capturing)); + expect(capturing.encoding, equals(utf8)); + expect(capturing.lineTerminator, equals('\n')); + expect(() => capturing.terminalColumns, throwsA(isA())); + expect(() => capturing.terminalLines, throwsA(isA())); + capturing.addError('ignored'); + await expectLater(capturing.flush(), completes); + await expectLater(capturing.close(), completes); + await expectLater(capturing.done, completes); + }); + }); + + group('sanitizeCommandOutput', () { + test('strips ANSI escape sequences', () { + expect( + sanitizeCommandOutput('\x1B[31mred\x1B[0m and \x1B[1mbold\x1B[0m'), + equals('red and bold'), + ); + }); + + test('normalizes CRLF to LF', () { + expect(sanitizeCommandOutput('a\r\nb\r\nc'), equals('a\nb\nc')); + }); + + test('collapses carriage-return redraws to the settled text', () { + // A spinner redrawing one line in place: erase + rewrite per tick. + expect( + sanitizeCommandOutput( + '\r\x1B[2K00:01 +1\r\x1B[2K00:02 +5\r\x1B[2KAll tests passed!', + ), + equals('All tests passed!'), + ); + }); + + test('preserves genuine newlines while collapsing redraws per line', () { + expect( + sanitizeCommandOutput('compiling...\rdone\nAll tests passed!'), + equals('done\nAll tests passed!'), + ); + }); + + test('trims trailing padding left by line erases', () { + expect(sanitizeCommandOutput('result '), equals('result')); + }); + + test('leaves plain multi-line output untouched', () { + expect(sanitizeCommandOutput('line 1\nline 2'), equals('line 1\nline 2')); + }); + }); +} From 7851a95ce5462c2194aacfe0583f601c08a33b75 Mon Sep 17 00:00:00 2001 From: Marcos Sevilla Date: Mon, 5 Oct 2026 18:52:45 +0300 Subject: [PATCH 4/5] refactor(mcp): drop the boolean parameter from _flag Only the optimization flag matched on false, so it is inlined as a collection-if instead of widening _flag with a whenValue parameter. Also points the _runToolCommand docs at ToolRun.capture rather than duplicating the Logger-in-zone rationale. --- lib/src/mcp/mcp_server.dart | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/lib/src/mcp/mcp_server.dart b/lib/src/mcp/mcp_server.dart index 1c23d715c..44498eb16 100644 --- a/lib/src/mcp/mcp_server.dart +++ b/lib/src/mcp/mcp_server.dart @@ -411,7 +411,7 @@ Only one value can be selected. 'test', ..._flag(args, 'coverage', '--coverage'), ..._flag(args, 'recursive', '-r'), - ..._flag(args, 'optimization', '--no-optimization', whenValue: false), + if (args['optimization'] == false) '--no-optimization', ..._option(args, 'concurrency', '-j'), ..._option(args, 'tags', '-t'), ..._option(args, 'exclude_coverage', '--exclude-coverage'), @@ -443,13 +443,12 @@ Only one value can be selected. ]; } - /// Returns `[flag]` when `args[key]` equals [whenValue], else nothing. + /// Returns `[flag]` when `args[key]` is `true`, else nothing. static List _flag( Map args, String key, - String flag, { - bool whenValue = true, - }) => args[key] == whenValue ? [flag] : const []; + String flag, + ) => args[key] == true ? [flag] : const []; /// Returns `[option, value]` when `args[key]` is set, else nothing. static List _option( @@ -556,9 +555,7 @@ Only one value can be selected. /// right package (`directory` is the working directory, not a positional /// argument). /// - /// The [Logger] is constructed *inside* the zone on purpose: mason captures - /// `IOOverrides.current` at [Logger] construction time, so building it - /// outside the zone would defeat the redirect. + /// See [ToolRun.capture] for why the [Logger] is built inside the zone. Future _runToolCommand( List args, { required String toolName, From abc4e53fb43456505b91844e61a512af4dee553f Mon Sep 17 00:00:00 2001 From: Marcos Sevilla Date: Fri, 9 Oct 2026 10:01:42 +0200 Subject: [PATCH 5/5] refactor(mcp): address review feedback on the test tool refactor Build _parseCreate from the shared _flag/_option helpers, keep failure construction private to ToolRun by mapping thrown exceptions through ToolRun.resultForException, and cover ToolRun directly in its own tests. --- lib/src/mcp/mcp_server.dart | 66 +++++----------- lib/src/mcp/tool_run.dart | 24 +++++- test/src/mcp/tool_run_test.dart | 131 +++++++++++++++++++++++++++++++- 3 files changed, 171 insertions(+), 50 deletions(-) diff --git a/lib/src/mcp/mcp_server.dart b/lib/src/mcp/mcp_server.dart index 44498eb16..2df3955ba 100644 --- a/lib/src/mcp/mcp_server.dart +++ b/lib/src/mcp/mcp_server.dart @@ -1,7 +1,6 @@ import 'dart:async'; import 'dart:io' show Directory, IOOverrides; -import 'package:args/command_runner.dart'; import 'package:dart_mcp/server.dart'; import 'package:mason/mason.dart' hide packageVersion; import 'package:meta/meta.dart'; @@ -363,41 +362,25 @@ Only one value can be selected. final subcommand = args['subcommand']! as String; final name = args['name']! as String; - final cliArgs = ['create', subcommand, name]; - - if (args['description'] != null) { - cliArgs.addAll(['--desc', args['description']! as String]); - } - if (args['org_name'] != null) { - cliArgs.addAll(['--org-name', args['org_name']! as String]); - } - if (args['output_directory'] != null) { - cliArgs.addAll(['-o', args['output_directory']! as String]); - } - if (args['application_id'] != null) { - cliArgs.addAll(['--application-id', args['application_id']! as String]); - } - if (args['platforms'] != null) { - cliArgs.addAll(['--platforms', args['platforms']! as String]); - } - if (args['publishable'] == true) { - cliArgs.add('--publishable'); - } - if (_workspaceSubcommands.contains(subcommand)) { - if (args['workspace'] == true) { - cliArgs.add('--workspace'); - } else if (args['workspace'] == false) { - cliArgs.add('--no-workspace'); - } - } - if (args['executable-name'] != null) { - cliArgs.addAll(['--executable-name', args['executable-name']! as String]); - } - if (args['template'] != null) { - cliArgs.addAll(['-t', args['template']! as String]); - } - - return cliArgs; + return [ + 'create', + subcommand, + name, + ..._option(args, 'description', '--desc'), + ..._option(args, 'org_name', '--org-name'), + ..._option(args, 'output_directory', '-o'), + ..._option(args, 'application_id', '--application-id'), + ..._option(args, 'platforms', '--platforms'), + ..._flag(args, 'publishable', '--publishable'), + if (_workspaceSubcommands.contains(subcommand)) + ...switch (args['workspace']) { + true => ['--workspace'], + false => ['--no-workspace'], + _ => const [], + }, + ..._option(args, 'executable-name', '--executable-name'), + ..._option(args, 'template', '-t'), + ]; } List _parseTest(Map args) { @@ -583,17 +566,8 @@ Only one value can be selected. (logger) => _commandRunnerBuilder(logger: logger).run(args), ); return run.resultFor(exitCode); - } on UsageException catch (e) { - return run.failure( - 'usage error: ${e.message}', - failureType: ToolFailureType.validation, - ); } on Exception catch (e, stackTrace) { - return run.failure( - 'threw an exception: $e', - failureType: ToolFailureType.transient, - stackTrace: stackTrace, - ); + return run.resultForException(e, stackTrace); } finally { if (directory != null) Directory.current = previousDirectory; } diff --git a/lib/src/mcp/tool_run.dart b/lib/src/mcp/tool_run.dart index 817a5b1bc..73fc158ca 100644 --- a/lib/src/mcp/tool_run.dart +++ b/lib/src/mcp/tool_run.dart @@ -2,6 +2,7 @@ import 'dart:async'; import 'dart:convert'; import 'dart:io' show IOOverrides, IOSink, Stdout, StdoutException, stderr; +import 'package:args/command_runner.dart'; import 'package:dart_mcp/server.dart'; import 'package:mason/mason.dart' hide packageVersion; import 'package:meta/meta.dart'; @@ -56,11 +57,30 @@ class ToolRun { /// Maps the command's [exitCode] to a success or failure result. CallToolResult resultFor(int exitCode) => exitCode == ExitCode.success.code ? _success() - : failure( + : _failure( 'failed with exit code $exitCode.', failureType: ToolFailureType.fromExitCode(exitCode), ); + /// Maps an [exception] thrown by the command to a failure result. + /// + /// A [UsageException] is a validation failure; anything else is transient + /// and logs its [stackTrace]. + CallToolResult resultForException( + Exception exception, + StackTrace stackTrace, + ) => switch (exception) { + UsageException(:final message) => _failure( + 'usage error: $message', + failureType: ToolFailureType.validation, + ), + _ => _failure( + 'threw an exception: $exception', + failureType: ToolFailureType.transient, + stackTrace: stackTrace, + ), + }; + CallToolResult _success() { final captured = _capturedOutput; return CallToolResult( @@ -78,7 +98,7 @@ class ToolRun { /// diagnostics emitted before a failure or throw are preserved. A short /// human-readable summary is also logged to the real stderr (the stdio /// transport forbids non-JSON on stdout, so stderr is free for diagnostics). - CallToolResult failure( + CallToolResult _failure( String reason, { required ToolFailureType failureType, StackTrace? stackTrace, diff --git a/test/src/mcp/tool_run_test.dart b/test/src/mcp/tool_run_test.dart index 5b106362a..9a33e4ca4 100644 --- a/test/src/mcp/tool_run_test.dart +++ b/test/src/mcp/tool_run_test.dart @@ -1,11 +1,138 @@ import 'dart:convert'; import 'dart:io'; +import 'package:args/command_runner.dart'; +import 'package:dart_mcp/server.dart'; +import 'package:mason/mason.dart' show ExitCode; import 'package:test/test.dart'; import 'package:very_good_cli/src/mcp/tool_run.dart'; void main() { - group('CapturingStdout', () { + group(ToolRun, () { + late StringBuffer stderrBuffer; + late ToolRun run; + + setUp(() { + stderrBuffer = StringBuffer(); + run = ToolRun( + toolName: 'test', + commandString: 'very_good test', + directory: '/tmp/project', + requestArguments: {'dart': true}, + ); + }); + + T withStderr(T Function() body) => + IOOverrides.runZoned(body, stderr: () => CapturingStdout(stderrBuffer)); + + String textOf(CallToolResult result, [int index = 0]) => + (result.content[index] as TextContent).text; + + Map jsonOf(CallToolResult result) => + jsonDecode(textOf(result)) as Map; + + group('capture', () { + test('returns the exit code of the command', () async { + final exitCode = await run.capture((_) async => 42); + expect(exitCode, equals(42)); + }); + + test('redirects stdout and logger output into the run', () async { + await run.capture((logger) async { + stdout.write('\x1B[31mfrom stdout\x1B[0m\n'); + logger.info('from logger'); + return ExitCode.success.code; + }); + + final result = run.resultFor(ExitCode.success.code); + expect(textOf(result, 1), equals('from stdout\nfrom logger')); + }); + }); + + group('resultFor', () { + test('returns a success result without output', () { + final result = run.resultFor(ExitCode.success.code); + + expect(result.isError, isFalse); + expect(result.content, hasLength(1)); + expect(textOf(result), equals('"test" completed successfully.')); + }); + + test('returns a structured failure for a non-zero exit code', () async { + await run.capture((logger) async { + logger.info('partial'); + return ExitCode.software.code; + }); + + final result = withStderr(() => run.resultFor(ExitCode.software.code)); + + expect(result.isError, isTrue); + expect( + jsonOf(result), + allOf( + containsPair('status', 'partial_failure'), + containsPair('failureType', 'business'), + containsPair('reason', 'failed with exit code 70.'), + containsPair('partialResults', 'partial'), + containsPair('attemptedAction', { + 'tool': 'test', + 'command': 'very_good test', + 'directory': '/tmp/project', + 'arguments': {'dart': true}, + }), + ), + ); + expect( + stderrBuffer.toString(), + equals( + '[very_good_mcp] "test" business error: ' + 'failed with exit code 70. (command: very_good test)\n', + ), + ); + }); + }); + + group('resultForException', () { + test('maps a UsageException to a validation failure', () { + final result = withStderr( + () => run.resultForException( + UsageException('bad flag', 'usage'), + StackTrace.empty, + ), + ); + + expect( + jsonOf(result), + allOf( + containsPair('failureType', 'validation'), + containsPair('reason', 'usage error: bad flag'), + ), + ); + expect(stderrBuffer.toString(), isNot(contains('Stack trace'))); + }); + + test('maps any other exception to a transient failure', () { + final stackTrace = StackTrace.current; + final result = withStderr( + () => run.resultForException(Exception('boom'), stackTrace), + ); + + expect( + jsonOf(result), + allOf( + containsPair('failureType', 'transient'), + containsPair('reason', 'threw an exception: Exception: boom'), + ), + ); + expect( + stderrBuffer.toString(), + contains('[very_good_mcp] Stack trace: $stackTrace'), + ); + }); + }); + }); + + group(CapturingStdout, () { late StringBuffer buffer; late CapturingStdout capturing; @@ -57,7 +184,7 @@ void main() { }); }); - group('sanitizeCommandOutput', () { + group(sanitizeCommandOutput, () { test('strips ANSI escape sequences', () { expect( sanitizeCommandOutput('\x1B[31mred\x1B[0m and \x1B[1mbold\x1B[0m'),