Repository navigation
refactor(mcp): reduce cognitive complexity of the test tool - #1758
marcossevilla wants to merge 10 commits into
Conversation
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.
|
Heads up — I'm auto-updating this PR by merging the latest |
|
Heads up — I'm auto-updating this PR by merging the latest |
|
Heads up — I'm auto-updating this PR by merging the latest |
|
Heads up — I'm auto-updating this PR by merging the latest |
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.
CapturingStdout and sanitizeCommandOutput now live in tool_run.dart, so their tests move to the matching test file.
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.
|
Heads up — I'm auto-updating this PR by merging the latest |
|
Heads up — I'm auto-updating this PR by merging the latest |
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