test: validate complete MCP CLI invocations - #362
YoungJinJung wants to merge 3 commits into
Conversation
- Parse mapped tool arguments against a fresh command tree. - Enforce positional, required flag, group, and JSON constraints.
|
Warning Review limit reachedNext included review available in 5 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (1)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📜 Recent review details⏰ Context from checks skipped due to timeout. (1)
🧰 Additional context used📓 Path-based instructions (4)Check that tests cover API errors, mapping edge cases, and navigation state transitions, not only happy paths.⚙️ CodeRabbit configuration file Files:
For Go reviews, look beyond compilation and prioritize nil pointer risks, context propagation, AWS SDK pagination, error wrapping, deterministic sorting, and stable table/detail rendering.⚙️ CodeRabbit configuration file Files:
Tests use mock client interfaces (see `rds_test.go` pattern) in Go test files📄 CodeRabbit inference engine (CLAUDE.md) Files:
Use lipgloss for styled TUI output — column-aligned tables with dimmed labels in Go implementation files Implement scroll windowing with formula: `visibleLines := max(m.height-N, 5)` in Go TUI implementation📄 CodeRabbit inference engine (CLAUDE.md) Files:
🔇 Additional comments (1)
WalkthroughThe MCP surface test now performs full CLI validation for each mapped resource tool. It checks command resolution, flag parsing, argument validation, required flags, flag groups, and the enabled ChangesMCP CLI validation
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Other Merge Risk: ⚪ Minimal · up to The change strengthens MCP dispatch validation without introducing a merge-blocking risk. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
/q review |
![amazon-q-developer[bot]](https://avatars.githubusercontent.com/in/1220912?s=60&v=4){kind=small}
There was a problem hiding this comment.
The validation logic correctly implements complete MCP CLI invocation testing. The added validation chain properly verifies command resolution, flag parsing, argument validation, required flags, flag groups, and JSON output requirements without executing AWS loaders. No blocking issues identified.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
There was a problem hiding this comment.
The validation logic correctly implements the requirements to resolve each mapped MCP invocation against a fresh CLI command tree and validate flags, positional arguments, required flags, and flag groups without executing AWS loaders. The code is well-structured with proper error handling and comprehensive validation checks. No blocking issues identified.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
YoungJinJung
left a comment
YoungJinJung
left a comment
There was a problem hiding this comment.
Findings
-
Medium — reject resolved commands that Cobra cannot execute (
internal/mcp/agent_surface_test.go:162)After
Find, the new chain validates flags, positional arguments, required flags, flag groups, and JSON mode, but it never checkscommand.Runnable(). Cobra returnsflag.ErrHelpbefore those validations when a command has neitherRunnorRunE, so a mappedresourceschild could keep the expected annotations and--jsonflag, pass this guard, and still fail every MCP call instead of returning JSON. Reject!command.Runnable()immediately after the successfulFind.
Reviewed head: 4f35c33
- Ensure mapped resource tools resolve to executable Cobra commands.
|
Addressed in |
|
/q review |
There was a problem hiding this comment.
The changes add comprehensive validation for MCP CLI invocations, including command discovery, flag parsing, argument validation, required flags, flag groups, and JSON output verification. The implementation is correct with proper error handling throughout the validation chain.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
YoungJinJung
left a comment
YoungJinJung
left a comment
There was a problem hiding this comment.
Re-reviewed head 120737f after the prior finding on 4f35c33. The new command.Runnable() guard closes the non-executable-command gap before flag validation. I reviewed the complete one-file diff and found no further actionable concerns. Local validation passed: make test and make build.
youngjinjung-linq
left a comment
youngjinjung-linq
left a comment
There was a problem hiding this comment.
Reviewed head 120737f after the runnable-command follow-up. I read the complete current diff, issue context, tests, CI, prior reviews, and discussion; no actionable concerns remain. Local validation passed: the targeted parity test, make test, and make build.
Redundant review submitted under the wrong authenticated identity; YoungJinJung had already reviewed this unchanged head.
youngjinjung-linq
left a comment
youngjinjung-linq
left a comment
There was a problem hiding this comment.
Re-reviewed head 120737f after the prior approval was dismissed. I read the complete current diff, issue context, tests, CI, prior reviews, comments, and thread state. The validation follows Cobra’s real command path without adding production complexity, and no actionable concerns remain. Local validation passed: the uncached parity test, make test, make build, and git diff --check.
2 participants
Summary
Related Issues
Part of #360
Follow-up to #361
Validation
Checklist
Summary by CodeRabbit
--json=truesetting.