Fix MCP server startup issues and add tools - #45
Conversation
📝 WalkthroughWalkthroughReorganizes README into categorized tool sections and expands documentation; adds a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~28 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 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 |
|
I seem unable to start it (Windows 10), so I almost rebuilt it and added some ideas that I thought were suitable for embedded development. In the end, I used AI to help me with the code structure and documentation improvements—I hope you don't mind. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@esp_utils.py`:
- Around line 11-46: run_command_async currently executes a shell string with
shell=True which is vulnerable to command injection when user-controlled values
(e.g., target, project_name, baud, partition_table from main.py) are
interpolated; fix by ensuring all user-supplied inputs are safely escaped before
building the command string (use shlex.quote() on each user value) or, better,
avoid shell=True entirely by reworking the call sites to pass a list of
arguments or pass user values via environment variables to subprocess.run
(update callers in main.py that build commands with f-strings to either quote
inputs with shlex.quote or change to argument lists/env and then call
run_command_async with those safe inputs while keeping the function
signature/behavior consistent).
- Around line 142-201: The call to subprocess.run(["mode"]) in list_serial_ports
can be hijacked on Windows; update the Windows branch in list_serial_ports to
construct a fully-qualified command path using
os.path.join(os.environ.get("SystemRoot", r"C:\Windows"), "System32",
"mode.com") and check os.path.exists on that path, using it as the first choice
for the command; if that file does not exist fall back to ["mode"] to preserve
compatibility. Ensure the subprocess.run invocation (the block that currently
uses ["mode"]) is replaced to use the chosen command variable and keep the same
capture_output/text/timeout settings and subsequent parsing logic.
🧹 Nitpick comments (1)
README.md (1)
20-86: Keep the tool list in sync with the actual registry.
With 30 tools, manual lists tend to drift. Consider generating this section from the tool registry or adding a lightweight check to keep docs aligned with the code.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@config.py`:
- Around line 86-123: The logger.warning calls that use f-strings without
placeholders should drop the unnecessary f prefix to satisfy Ruff F541; locate
the warnings in the config loader where environment parsing sets
config.SERIAL_PORT_TIMEOUT, config.DEFAULT_FLASH_BAUD,
config.DEFAULT_MONITOR_BAUD, config.MAX_ERRORS, and config.MAX_WARNINGS and
replace strings like f"Invalid ESP_MCP_SERIAL_TIMEOUT value, using default" with
plain string literals (remove the leading f) while leaving the surrounding
try/except and logger.debug lines unchanged.
- Around line 80-105: The env-parsing blocks for DEFAULT_COMMAND_TIMEOUT,
SERIAL_PORT_TIMEOUT, DEFAULT_FLASH_BAUD, and DEFAULT_MONITOR_BAUD should
validate that the parsed int is > 0 before assigning; if the value is <= 0 or
raises ValueError, keep the existing default and call logger.warning with a
descriptive message. Update the try/except blocks around parsing of
ESP_MCP_TIMEOUT, ESP_MCP_SERIAL_TIMEOUT, ESP_MCP_FLASH_BAUD, and
ESP_MCP_MONITOR_BAUD to check the parsed value > 0, assign only on success, and
log the fallback when invalid so subprocess.run(timeout=...) and serial/baud
settings never receive non-positive integers.
In `@esp_mcp.egg-info/PKG-INFO`:
- Around line 1-5: The package metadata contains a placeholder description ("Add
your description here") in the description field of pyproject.toml; replace that
placeholder with a concise, meaningful package description describing esp-mcp's
purpose and functionality (e.g., what it provides, its main responsibility),
updating the description key in pyproject.toml so the package metadata
(Metadata-Version / Name: esp-mcp / Version: 0.1.0) no longer contains the
placeholder.
🧹 Nitpick comments (3)
config.py (1)
5-34: Annotate mutable class attributes as ClassVar (or make immutable).
COMMON_SERIAL_PORTSis a mutable list; marking it asClassVarclarifies intent for type checkers and avoids instance shadowing. Consider a tuple if you want immutability.♻️ Suggested change
-from typing import List, Dict, Any +from typing import List, Dict, Any, ClassVar @@ - COMMON_SERIAL_PORTS = [ + COMMON_SERIAL_PORTS: ClassVar[List[str]] = [ "COM1", "COM2", "COM3", "COM4", "COM5", "COM6", "/dev/ttyUSB0", "/dev/ttyUSB1", "/dev/ttyACM0", "/dev/ttyACM1", "/dev/cu.usbserial-*", "/dev/cu.SLAB_USBtoUART" ]esp_utils.py (1)
52-57: Make Optional parameters explicit in type hints.
idf_pathdefaults toNone; update the annotation tostr | None(and mirror the change inget_export_scriptandcheck_esp_idf_installed) to satisfy PEP 484 tooling.♻️ Suggested change
-def get_esp_idf_dir(idf_path: str = None) -> str: +def get_esp_idf_dir(idf_path: str | None = None) -> str:esp_mcp.egg-info/requires.txt (1)
1-2: Move pytest to an optional extra since it's only used as a CLI tool, not a runtime dependency.pytest is invoked as an external command via subprocess in main.py, not imported as a Python module. Since there are no test files in the repo and pytest is only executed as a CLI tool for users who need that feature, it should be specified as an optional extra (e.g.,
mcp[cli,pytest]) rather than a hard runtime dependency. This avoids requiring pytest for all users.
1 participant
Dear @horw,
I fixed some problems I found when using this MCP server and added some tools.
Issues Fixed
1. Server Startup Failures
2. Platform Compatibility
3. ESP-IDF Path Detection
New Tools Added (23 tools, 30 total)
Project Management
get_project_info: Get detailed project informationlist_components: List all components in projectBuild & Flash
clean_esp_project: Clean build fileserase_flash_esp: Erase flash memoryflash_and_monitor_esp: Flash and immediately monitormenuconfig_esp: Run menuconfigConfiguration
get_project_config: Get sdkconfig informationset_esp_partition: Set partition tableget_esp_idf_version: Get ESP-IDF versioncheck_esp_idf_env: Check environment statusDebugging
gdb_attach: Attach GDB debuggerget_core_dump: Get core dump informationRuntime Analysis
get_heap_info: Get heap memory infoget_task_stats: Get FreeRTOS task statisticsFile Operations
read_file: Read file contentswrite_file: Write content to filelist_files: List files and directoriesAnalysis Tools
parse_build_log: Structured build log analysisanalyze_memory_map: Memory usage analysis from .map filescompare_sdkconfig: Compare sdkconfig differencesanalyze_dependencies: Analyze component dependenciesformat_device_log: Format device serial logsDocumentation Updates
Notes
Thank you for creating this MCP server."
Summary by CodeRabbit
Documentation
New Features
Bug Fixes
Chores