diff --git a/lib/src/mcp/mcp_server.dart b/lib/src/mcp/mcp_server.dart index 7d698ac5c..c42dec75b 100644 --- a/lib/src/mcp/mcp_server.dart +++ b/lib/src/mcp/mcp_server.dart @@ -10,6 +10,7 @@ import 'package:meta/meta.dart'; import 'package:stream_channel/stream_channel.dart'; import 'package:very_good_cli/src/command_runner.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/version.dart'; /// {@template command_runner_builder} @@ -41,10 +42,8 @@ final class VeryGoodMCPServer extends MCPServer with ToolsSupport { /// {@macro very_good_mcp_server} VeryGoodMCPServer({ required StreamChannel channel, - CommandRunnerBuilder? commandRunnerBuilder, - }) : _commandRunnerBuilder = - commandRunnerBuilder ?? defaultCommandRunnerBuilder, - super.fromStreamChannel( + this._commandRunnerBuilder = defaultCommandRunnerBuilder, + }) : super.fromStreamChannel( channel, implementation: Implementation( name: 'very_good_cli', @@ -484,7 +483,7 @@ Only one value can be selected. Future _handleCreate(CallToolRequest request) async { final args = request.arguments ?? {}; final cliArgs = _parseCreate(args); - return _runToolCommand(cliArgs, toolName: 'create'); + return _runToolCommand(cliArgs, toolName: 'create', requestArguments: args); } Future _handleTest(CallToolRequest request) async { @@ -494,6 +493,7 @@ Only one value can be selected. cliArgs, toolName: 'test', directory: args['directory'] as String?, + requestArguments: args, ); } @@ -504,6 +504,7 @@ Only one value can be selected. cliArgs, toolName: 'packages get', directory: args['directory'] as String?, + requestArguments: args, ); } @@ -515,16 +516,14 @@ Only one value can be selected. final checkLicenses = args['licenses'] as bool? ?? true; if (!checkLicenses) { - return CallToolResult( - content: [ - TextContent( - text: - 'No check specified. Currently only "licenses" check is ' - 'supported. Set licenses=true to run license checks.', - ), - ], - isError: true, - ); + return StructuredToolError( + toolName: 'packages check licenses', + reason: + 'No check specified. Currently only "licenses" check is ' + 'supported. Set licenses=true to run license checks.', + failureType: ToolFailureType.validation, + attemptedArguments: args, + ).toCallToolResult(); } final cliArgs = _parsePackagesCheck(args); @@ -532,6 +531,7 @@ Only one value can be selected. cliArgs, toolName: 'packages check licenses', directory: args['directory'] as String?, + requestArguments: args, ); } @@ -556,6 +556,7 @@ Only one value can be selected. List args, { required String toolName, String? directory, + Map? requestArguments, }) { return _lock.run(() async { final commandString = 'very_good ${args.join(' ')}'; @@ -570,31 +571,34 @@ Only one value can be selected. ); } - // Appends the captured command output (the real diagnostics) to a - // message, on every result path so partial output emitted before a throw - // is not lost. The buffer is populated whether the run returns or throws. - String withCapturedOutput(String message) { + // 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(); - if (captured.isEmpty) return message; - return '$message\n\nOutput:\n$captured'; - } - - // Builds a failure result from [reason] (the human-readable cause). The - // message is logged once to the real stderr (the stdio transport forbids - // only non-JSON on stdout, so stderr is free for diagnostics) and also - // surfaced — with any captured output — in the tool result, so the same - // text never has to be written twice. [commandString] is appended to keep - // the failure reproducible. - CallToolResult errorResult(String reason, {StackTrace? stackTrace}) { - final message = '"$toolName" $reason\nCommand: $commandString'; - stderr.writeln('[very_good_mcp] ${message.replaceAll('\n', ' ')}'); + stderr.writeln( + '[very_good_mcp] "$toolName" ${failureType.name} error: $reason ' + '(command: $commandString)', + ); if (stackTrace != null) { stderr.writeln('[very_good_mcp] Stack trace: $stackTrace'); } - return CallToolResult( - content: [TextContent(text: withCapturedOutput(message))], - isError: true, - ); + return StructuredToolError( + toolName: toolName, + reason: reason, + failureType: failureType, + commandString: commandString, + directory: directory, + attemptedArguments: requestArguments, + capturedOutput: captured, + ).toCallToolResult(); } // Apply [directory] as the real working directory for the duration of @@ -621,11 +625,21 @@ Only one value can be selected. ); } - return errorResult('failed with exit code $exitCode.'); + return errorResult( + 'failed with exit code $exitCode.', + failureType: ToolFailureType.fromExitCode(exitCode), + ); } on UsageException catch (e) { - return errorResult('usage error: ${e.message}'); + return errorResult( + 'usage error: ${e.message}', + failureType: ToolFailureType.validation, + ); } on Exception catch (e, stackTrace) { - return errorResult('threw an exception: $e', stackTrace: stackTrace); + return errorResult( + 'threw an exception: $e', + failureType: ToolFailureType.transient, + stackTrace: stackTrace, + ); } finally { if (directory != null) Directory.current = previousDirectory; } @@ -697,11 +711,14 @@ class CapturingStdout implements Stdout { bool get supportsAnsiEscapes => false; @override - int get terminalColumns => - throw const StdoutException('No terminal attached'); + int get terminalColumns { + throw const StdoutException('No terminal attached'); + } @override - int get terminalLines => throw const StdoutException('No terminal attached'); + int get terminalLines { + throw const StdoutException('No terminal attached'); + } @override IOSink get nonBlocking => this; @@ -728,9 +745,9 @@ String sanitizeCommandOutput(String raw) { .replaceAll(_ansiEscape, '') .replaceAll('\r\n', '\n') .split('\n') - .map( - (line) => - (line.contains('\r') ? line.split('\r').last : line).trimRight(), - ) + .map((line) { + final output = line.contains('\r') ? line.split('\r').last : line; + return output.trimRight(); + }) .join('\n'); } diff --git a/lib/src/mcp/structured_tool_error.dart b/lib/src/mcp/structured_tool_error.dart new file mode 100644 index 000000000..fb1f2b992 --- /dev/null +++ b/lib/src/mcp/structured_tool_error.dart @@ -0,0 +1,174 @@ +import 'dart:convert'; + +import 'package:dart_mcp/server.dart'; +import 'package:mason/mason.dart' hide packageVersion; +import 'package:meta/meta.dart'; + +/// Classifies a tool failure into a bucket an agent loop can act on without +/// parsing free-form text. +/// +/// * `validation` — the caller supplied bad input; retrying as-is won't help. +/// * `permission` — the process lacks filesystem or credential access. +/// * `transient` — an environment or infrastructure hiccup; retrying may +/// succeed. +/// * `business` — a domain rule was violated; the safest default for an +/// outcome that can't be attributed to the other three. +enum ToolFailureType { + /// A caller-supplied argument was invalid. + validation, + + /// The process lacked permission to complete the action. + permission, + + /// An environment or infrastructure hiccup; retrying may resolve it. + transient, + + /// A domain rule was violated. + business; + + /// Classifies an [exitCode] into a [ToolFailureType]. + /// + /// Codes follow the sysexits.h conventions surfaced by `package:io`'s + /// [ExitCode]; unknown codes fall back to [ToolFailureType.business], the + /// safest default for an outcome we can't attribute to a transient failure + /// or a bad input. + factory ToolFailureType.fromExitCode(int exitCode) { + if (exitCode == ExitCode.usage.code || + exitCode == ExitCode.data.code || + exitCode == ExitCode.noInput.code || + exitCode == ExitCode.config.code) { + return ToolFailureType.validation; + } + + if (exitCode == ExitCode.noPerm.code) { + return ToolFailureType.permission; + } + + if (exitCode == ExitCode.unavailable.code || + exitCode == ExitCode.tempFail.code || + exitCode == ExitCode.ioError.code || + exitCode == ExitCode.osError.code || + exitCode == ExitCode.osFile.code || + exitCode == ExitCode.cantCreate.code) { + return ToolFailureType.transient; + } + + return ToolFailureType.business; + } +} + +/// Suggests alternative approaches keyed on [failureType]. +/// +/// These are surfaced verbatim in the structured error payload so an agent +/// coordinator has recovery options attached to every failure without having +/// to reason about the failure type itself. +@visibleForTesting +List alternativeApproachesFor(ToolFailureType failureType) { + const alternativeApproachesByFailureType = { + ToolFailureType.transient: [ + 'Retry the command; the failure may resolve on its own.', + 'Check network and remote service availability, then retry.', + 'If retries keep failing with the same error, switch approach.', + ], + ToolFailureType.validation: [ + 'Correct any invalid tool arguments before retrying.', + 'Consult the tool schema for accepted parameters and values.', + 'Inspect the captured output for the field the CLI rejected.', + ], + ToolFailureType.permission: [ + 'Ensure the process can read and write the target directory.', + 'Retry after adjusting filesystem permissions or credentials.', + 'Cannot be retried as-is without an authorization change.', + ], + ToolFailureType.business: [ + 'Inspect the captured output for the specific rule reported.', + 'Try an alternate subcommand, template, or configuration.', + 'Escalate to the user if the constraint cannot be satisfied.', + ], + }; + + return alternativeApproachesByFailureType[failureType]!; +} + +/// {@template structured_tool_error} +/// A structured description of a tool failure, renderable as a +/// [CallToolResult]. +/// +/// Encapsulates the JSON payload shape agents rely on to pick a recovery +/// strategy: +/// +/// * `status` — `partial_failure` if [capturedOutput] is non-empty, else +/// `failure`. +/// * `failureType` — the [ToolFailureType] name. +/// * `attemptedAction` — the tool name, the concrete CLI command that was +/// invoked (when known), the working directory (when set), and the raw +/// caller-supplied arguments. +/// * `reason` — the human-readable failure cause. +/// * `partialResults` — the sanitized captured command output, when any. +/// * `alternativeApproaches` — recovery suggestions from +/// [alternativeApproachesFor]. +/// {@endtemplate} +class StructuredToolError { + /// {@macro structured_tool_error} + const StructuredToolError({ + required this.toolName, + required this.reason, + required this.failureType, + this.commandString, + this.directory, + this.capturedOutput, + this.attemptedArguments, + }); + + /// The name of the tool that failed. + final String toolName; + + /// The human-readable failure cause. + final String reason; + + /// The bucket this failure falls into. + final ToolFailureType failureType; + + /// The concrete CLI command that was invoked, when known. + final String? commandString; + + /// The working directory the command ran in, when set. + final String? directory; + + /// The sanitized captured command output, when any. + final String? capturedOutput; + + /// The raw caller-supplied arguments. + final Map? attemptedArguments; + + bool get _hasPartialResults => + capturedOutput != null && capturedOutput!.isNotEmpty; + + /// The JSON payload describing this failure. + Map toJson() { + final attemptedAction = { + 'tool': toolName, + 'command': ?commandString, + 'directory': ?directory, + if (attemptedArguments != null && attemptedArguments!.isNotEmpty) + 'arguments': attemptedArguments, + }; + + return { + 'status': _hasPartialResults ? 'partial_failure' : 'failure', + 'failureType': failureType.name, + 'attemptedAction': attemptedAction, + 'reason': reason, + if (_hasPartialResults) 'partialResults': capturedOutput, + 'alternativeApproaches': alternativeApproachesFor(failureType), + }; + } + + /// Renders this failure as a [CallToolResult] whose single text content is + /// the pretty-printed [toJson] payload. + CallToolResult toCallToolResult() { + final text = const JsonEncoder.withIndent(' ').convert(toJson()); + + return CallToolResult(content: [TextContent(text: text)], isError: true); + } +} diff --git a/test/src/mcp/mcp_server_test.dart b/test/src/mcp/mcp_server_test.dart index 6c36ce9a3..dc79aca22 100644 --- a/test/src/mcp/mcp_server_test.dart +++ b/test/src/mcp/mcp_server_test.dart @@ -11,6 +11,12 @@ import 'package:test/test.dart'; import 'package:very_good_cli/src/command_runner.dart'; import 'package:very_good_cli/src/mcp/mcp_server.dart'; +/// Decodes a MCP tool result's text content as the structured error payload. +Map _errorPayload(CallToolResult result) { + final text = (result.content.first as TextContent).text; + return jsonDecode(text) as Map; +} + class _MockVeryGoodCommandRunner extends Mock implements VeryGoodCommandRunner {} @@ -306,9 +312,24 @@ void main() { response['result'] as Map, ); expect(result.isError, isTrue); + final payload = _errorPayload(result); + expect(payload['status'], equals('failure')); + expect(payload['failureType'], equals('business')); + expect(payload['reason'], contains('failed with exit code 70')); + final action = payload['attemptedAction']! as Map; + expect(action['tool'], equals('create')); + expect(action['command'], contains('very_good create flutter_app')); expect( - (result.content.first as TextContent).text, - contains('"create" failed with exit code'), + action['arguments'], + equals({'subcommand': 'flutter_app', 'name': 'my_app'}), + ); + expect( + payload['alternativeApproaches'], + isA>().having( + (l) => l.length, + 'length', + greaterThan(0), + ), ); }); @@ -480,10 +501,9 @@ void main() { response['result'] as Map, ); expect(result.isError, isTrue); - expect( - (result.content.first as TextContent).text, - contains('"test" failed with exit code'), - ); + final payload = _errorPayload(result); + expect(payload['reason'], contains('failed with exit code 70')); + expect(payload['failureType'], equals('business')); }); test('passes --timeout when timeout_seconds is provided', () async { @@ -610,10 +630,13 @@ void main() { response['result'] as Map, ); expect(result.isError, isTrue); - expect( - (result.content.first as TextContent).text, - contains('No check specified'), - ); + final payload = _errorPayload(result); + expect(payload['status'], equals('failure')); + expect(payload['failureType'], equals('validation')); + expect(payload['reason'], contains('No check specified')); + final action = payload['attemptedAction']! as Map; + expect(action['tool'], equals('packages check licenses')); + expect(action['arguments'], equals({'licenses': false})); verifyNever(() => mockCommandRunner.run(any())); }); }); @@ -639,9 +662,11 @@ void main() { ); expect(result.isError, isTrue); - final text = (result.content.first as TextContent).text; - expect(text, contains('"create" usage error: bad usage')); - expect(text, contains('Command: very_good')); + final payload = _errorPayload(result); + expect(payload['failureType'], equals('validation')); + expect(payload['reason'], contains('usage error: bad usage')); + final action = payload['attemptedAction']! as Map; + expect(action['command'], contains('very_good')); }); test('handles general Exception with descriptive message', () async { @@ -664,10 +689,12 @@ void main() { ); expect(result.isError, isTrue); - final text = (result.content.first as TextContent).text; - expect(text, contains('"create" threw an exception')); - expect(text, contains('big bad')); - expect(text, contains('Command: very_good')); + final payload = _errorPayload(result); + expect(payload['failureType'], equals('transient')); + expect(payload['reason'], contains('threw an exception')); + expect(payload['reason'], contains('big bad')); + final action = payload['attemptedAction']! as Map; + expect(action['command'], contains('very_good')); }); }); @@ -735,10 +762,13 @@ void main() { response['result'] as Map, ); expect(result.isError, isTrue); - final text = (result.content.first as TextContent).text; - expect(text, contains('"test" failed with exit code 69')); - expect(text, contains('compile error: boom')); - expect(text, contains('stderr detail')); + final payload = _errorPayload(result); + expect(payload['status'], equals('partial_failure')); + expect(payload['failureType'], equals('transient')); + expect(payload['reason'], contains('failed with exit code 69')); + final partial = payload['partialResults']! as String; + expect(partial, contains('compile error: boom')); + expect(partial, contains('stderr detail')); }); test('includes captured output in a success result', () async { @@ -787,9 +817,10 @@ void main() { final result = CallToolResult.fromMap( response['result'] as Map, ); - final text = (result.content.first as TextContent).text; - expect(text, contains('stdout via logger')); - expect(text, contains('stderr via logger')); + final payload = _errorPayload(result); + final partial = payload['partialResults']! as String; + expect(partial, contains('stdout via logger')); + expect(partial, contains('stderr via logger')); }); test('includes captured output when the run throws', () async { @@ -807,9 +838,13 @@ void main() { response['result'] as Map, ); expect(result.isError, isTrue); - final text = (result.content.first as TextContent).text; - expect(text, contains('"test" threw an exception')); - expect(text, contains('partial output before crash')); + final payload = _errorPayload(result); + expect(payload['status'], equals('partial_failure')); + expect(payload['reason'], contains('threw an exception')); + expect( + payload['partialResults'], + contains('partial output before crash'), + ); }); test('omits the output block when nothing was captured', () async { diff --git a/test/src/mcp/structured_tool_error_test.dart b/test/src/mcp/structured_tool_error_test.dart new file mode 100644 index 000000000..caf547502 --- /dev/null +++ b/test/src/mcp/structured_tool_error_test.dart @@ -0,0 +1,150 @@ +import 'dart:convert'; + +import 'package:dart_mcp/server.dart'; +import 'package:mason/mason.dart'; +import 'package:test/test.dart'; +import 'package:very_good_cli/src/mcp/structured_tool_error.dart'; + +void main() { + group(ToolFailureType, () { + test('classifies validation exit codes', () { + expect( + ToolFailureType.fromExitCode(ExitCode.usage.code), + equals(ToolFailureType.validation), + ); + expect( + ToolFailureType.fromExitCode(ExitCode.data.code), + equals(ToolFailureType.validation), + ); + expect( + ToolFailureType.fromExitCode(ExitCode.noInput.code), + equals(ToolFailureType.validation), + ); + expect( + ToolFailureType.fromExitCode(ExitCode.config.code), + equals(ToolFailureType.validation), + ); + }); + + test('classifies permission exit codes', () { + expect( + ToolFailureType.fromExitCode(ExitCode.noPerm.code), + equals(ToolFailureType.permission), + ); + }); + + test('classifies transient exit codes', () { + expect( + ToolFailureType.fromExitCode(ExitCode.unavailable.code), + equals(ToolFailureType.transient), + ); + expect( + ToolFailureType.fromExitCode(ExitCode.tempFail.code), + equals(ToolFailureType.transient), + ); + expect( + ToolFailureType.fromExitCode(ExitCode.ioError.code), + equals(ToolFailureType.transient), + ); + expect( + ToolFailureType.fromExitCode(ExitCode.osError.code), + equals(ToolFailureType.transient), + ); + expect( + ToolFailureType.fromExitCode(ExitCode.osFile.code), + equals(ToolFailureType.transient), + ); + expect( + ToolFailureType.fromExitCode(ExitCode.cantCreate.code), + equals(ToolFailureType.transient), + ); + }); + + test('defaults unknown or software exit codes to business', () { + expect( + ToolFailureType.fromExitCode(ExitCode.software.code), + equals(ToolFailureType.business), + ); + expect( + ToolFailureType.fromExitCode(1), + equals(ToolFailureType.business), + ); + expect( + ToolFailureType.fromExitCode(255), + equals(ToolFailureType.business), + ); + }); + }); + + group('alternativeApproachesFor', () { + test('returns non-empty suggestions for every known failureType', () { + for (final type in ToolFailureType.values) { + expect( + alternativeApproachesFor(type), + isNotEmpty, + reason: '$type must offer at least one alternative approach', + ); + } + }); + }); + + group(StructuredToolError, () { + test('emits a "failure" status when no captured output is provided', () { + final result = const StructuredToolError( + toolName: 'test', + reason: 'boom', + failureType: ToolFailureType.business, + ).toCallToolResult(); + expect(result.isError, isTrue); + final payload = + jsonDecode((result.content.first as TextContent).text) + as Map; + expect(payload['status'], equals('failure')); + expect(payload['reason'], equals('boom')); + expect(payload['partialResults'], isNull); + final action = payload['attemptedAction']! as Map; + expect(action, equals({'tool': 'test'})); + }); + + test( + 'emits a "partial_failure" status and preserves captured output', + () { + final result = const StructuredToolError( + toolName: 'create', + reason: 'crashed', + failureType: ToolFailureType.transient, + commandString: 'very_good create flutter_app my_app', + directory: '/tmp/x', + attemptedArguments: {'name': 'my_app'}, + capturedOutput: 'compile error', + ).toCallToolResult(); + final payload = + jsonDecode((result.content.first as TextContent).text) + as Map; + expect(payload['status'], equals('partial_failure')); + expect(payload['partialResults'], equals('compile error')); + final action = payload['attemptedAction']! as Map; + expect( + action['command'], + equals('very_good create flutter_app my_app'), + ); + expect(action['directory'], equals('/tmp/x')); + expect(action['arguments'], equals({'name': 'my_app'})); + }, + ); + + test('omits arguments key when attemptedArguments is empty', () { + final result = const StructuredToolError( + toolName: 'test', + reason: 'oops', + failureType: ToolFailureType.business, + attemptedArguments: {}, + ).toCallToolResult(); + final payload = + jsonDecode((result.content.first as TextContent).text) + as Map; + final action = payload['attemptedAction']! as Map; + expect(action.containsKey('arguments'), isFalse); + }); + }); +}