-
Notifications
You must be signed in to change notification settings - Fork 11
Add maxResults parameter and return text results by default #14
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
To avoid errors like: Error: MCP tool "find_code" response (194537 tokens) exceeds maximum allowed tokens (25000). Please use pagination, filtering, or limit parameters to reduce the response size.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds maxResults
parameter and text output format support to prevent token limit errors that could occur when using the MCP server. The changes introduce pagination capabilities and a more token-efficient text format alongside the existing JSON format.
Key changes:
- Added
max_results
parameter tofind_code
andfind_code_by_rule
functions - Added
output_format
parameter with "text" as default (more token-efficient) and "json" options - Added comprehensive unit and integration tests with CI workflow
Reviewed Changes
Copilot reviewed 7 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
main.py | Added pagination parameters and text output format to core search functions |
tests/test_main.py | Comprehensive unit tests for all MCP server functions with mocking |
tests/test_integration.py | Integration tests using actual fixture files |
tests/fixtures/example.py | Test fixture with Python code examples |
pyproject.toml | Added development dependencies and testing configuration |
.github/workflows/test.yml | CI workflow for cross-platform testing |
README.md | Updated documentation for new parameters |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update.
Some comments are left.
# Apply max_results limit if specified | ||
lines = output.split('\n') | ||
non_empty_lines = [line for line in lines if line.strip()] | ||
if max_results is not None and len(non_empty_lines) > max_results: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
using lines isn't accurate here since a match can span multiple lines.
To make things easier I think the argument name can be changed to max_matching_lines
or simialr
output = '\n'.join(non_empty_lines) | ||
header = f"Found {len(non_empty_lines)} matches (limited to {max_results}):\n" | ||
else: | ||
header = f"Found {len(non_empty_lines)} matches:\n" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
similarly, the header can be omitted.
# Limit the results | ||
non_empty_lines = non_empty_lines[:max_results] | ||
output = '\n'.join(non_empty_lines) | ||
header = f"Found {len(non_empty_lines)} matches (limited to {max_results}):\n" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be a single line of "Showing the {max_len} lines of results"
To avoid errors like:
The first commit b28e40d addresses the problem.
The second commits 805bd67 adds tests and a CI workflow for the mcp tools.