refactor(mcp): reduce cognitive complexity of the test tool - #1758
marcossevilla wants to merge 1 commit into
Conversation
| /// | ||
| /// Owns the buffer that captures the command's output and turns the outcome | ||
| /// of the run into a [CallToolResult]. | ||
| class _ToolRun { |
There was a problem hiding this comment.
move this to another file
| 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); |
There was a problem hiding this comment.
make run.capture return the exitCode directly
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
13ec12e to
ab446c8
Compare
|
@vgvbot /review-pr |
|
Code review submitted. |
![unicoderbot[bot]](https://avatars.githubusercontent.com/in/3144111?s=60&v=4){kind=small}
There was a problem hiding this comment.
The refactor cleanly reduces cognitive complexity in both _parseTest and _runToolCommand. The _flag/_option helpers preserve argument order and null/type handling, and the _ToolRun extraction keeps stderr messages and StructuredToolError fields intact.
However, there is a critical bug: the _ToolRun constructor is declared as new({...}) (line 615) with no class-name prefix. This is not valid Dart constructor syntax — a constructor must begin with the type identifier (e.g. _ToolRun({...}) or _ToolRun.new({...})). The call site at line 571 uses _ToolRun(toolName: ...) and expects an unnamed constructor, so this file will fail to compile. The description claims no new tests were added because existing tests already cover the branches, so this would surface the first time the test suite is run.
| /// Owns the buffer that captures the command's output and turns the outcome | ||
| /// of the run into a [CallToolResult]. | ||
| class _ToolRun { | ||
| new({ |
There was a problem hiding this comment.
This is not a valid Dart constructor declaration. A constructor must start with the class name — either _ToolRun({...}) (unnamed) or _ToolRun.new({...}) (explicit .new alias). Written as just new({...}), this file will not compile, and since _ToolRun is instantiated as _ToolRun(toolName: ...) at line 571 (an unnamed-constructor call), the implicit default constructor it would otherwise fall back to also wouldn't match. Change to _ToolRun({ required this.toolName, required this.commandString, required this.directory, required this.requestArguments });.
This branch has not been deployed
1 participant
Status
READY
Description
_parseTestbuilds its argument list in one expression from_flagand_optionhelpers, keeping the exact order of the original if-chain._runToolCommandhands its captured output buffer and its success and failure builders to a private_ToolRun, so the method only sets the working directory, runs the command and maps exceptions to failures. Stderr messages andStructuredToolErrorfields are unchanged.timeout_secondsandpathsare now read at the top of_parseTest. Well-formed input produces the same arguments, and badly typed input still throws aTypeError.Found by the full cognitive complexity scan in #1755, which tests VeryGoodOpenSource/very_good_workflows#520. Every function in the touched files now scores 15 or under. Behavior, log output, exit codes and public APIs are unchanged, and no tests were needed because the existing ones already reach every new branch.
VeryGoodMCPServer._parseTestVeryGoodMCPServer._runToolCommandPart of a three-PR cleanup. Once all three merge, the full scan on #1755 passes:
Type of Change
🤖 Generated with Claude Code