feat(mcp): Add <idf.py monitor> support for MCP#18385
Conversation
|
|
👋 Hello jinw06k, we appreciate your contribution to this project! 📘 Please review the project's Contributions Guide for key guidelines on code, documentation, testing, and more. 🖊️ Please also make sure you have read and signed the Contributor License Agreement for this project. Click to see more instructions ...
Review and merge process you can expect ...
|
jinw06k
changed the title
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
| cmd, stdin=slave_fd, stdout=slave_fd, stderr=slave_fd, | ||
| cwd=cwd, start_new_session=True, | ||
| ) | ||
| os.close(slave_fd) |
There was a problem hiding this comment.
PTY file descriptors leak if Popen fails
Medium Severity
If subprocess.Popen raises an exception (e.g., OSError from an invalid cwd, permissions, etc.), both master_fd and slave_fd from _pty_open_noecho() are leaked. The os.close(slave_fd) call and self._master_fd = master_fd assignment sit after the Popen call with no try/finally guard, so neither fd is ever closed on failure. The same pattern appears in monitor_boot. Repeated failures would accumulate leaked file descriptors.
Additional Locations (1)
| self._master_fd = None | ||
| if self._reader_thread is not None: | ||
| self._reader_thread.join(timeout=3) | ||
| self._reader_thread = None |
There was a problem hiding this comment.
Reader thread crashes on TypeError, loses buffered data
Low Severity
In stop(), the reader thread is joined after self._master_fd is set to None (lines 236–237 or 252–257). A race exists where the reader thread passes the while not self._stop_event.is_set() check, then stop() closes the fd and sets self._master_fd = None, then the reader calls select.select([self._master_fd], ...) with None. This raises TypeError, which is not caught by except OSError on line 163. The uncaught exception skips the partial-buffer flush at lines 166–168, silently losing any incomplete line data in buf.
Additional Locations (1)
|
You have used all of your free Bugbot PR reviews. To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial. |
|
You have used all of your free Bugbot PR reviews. To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial. |
| deadline = time.monotonic() + timeout if timeout > 0 else 0 | ||
|
|
||
| while True: | ||
| lines, remaining = monitor_session.read(max_lines=max_lines) |
There was a problem hiding this comment.
monitor_read can return more than max_lines lines
The polling loop passes the original max_lines to every read() call without decrementing it by the lines already collected. When timeout > 0, each iteration can fetch up to max_lines additional lines, so the total may significantly exceed the caller's requested limit.
Suggested fix:
budget = max_lines
while True:
lines, remaining = monitor_session.read(max_lines=budget)
if lines:
collected.extend(lines)
budget -= len(lines)
if wait_for and wait_for in ''.join(collected):
break
if budget <= 0:
break
if deadline == 0:
break
if time.monotonic() >= deadline:
break
if not monitor_session.is_running:
break
time.sleep(0.3)| if not chunk: | ||
| break | ||
| parts.append(chunk.decode('utf-8', errors='replace')) | ||
| if wait_for and wait_for in ''.join(parts): |
There was a problem hiding this comment.
O(n²) join for wait_for check in _pty_read_for
On each received chunk, ''.join(parts) re-joins the entire accumulated output to scan for wait_for. For long captures this is quadratic in total output size.
A simple fix is to check only the tail of the buffer — the match can only span the boundary between the previous accumulated text and the new chunk:
parts.append(chunk.decode('utf-8', errors='replace'))
if wait_for:
# Only the last (len(wait_for)-1 + new chunk) bytes can contain a new match
tail = ''.join(parts)[-(len(wait_for) + len(parts[-1])):]
if wait_for in tail:
breakOr maintain a running concatenated string instead of a list of parts.
| except ImportError: | ||
| _PTY_SUPPORTED = False | ||
|
|
||
| _ANSI_RE = re.compile(r'\x1b\[[0-9;]*[A-Za-z]') |
There was a problem hiding this comment.
Incomplete ANSI escape sequence stripping
The current regex \x1b\[[0-9;]*[A-Za-z] only handles CSI sequences. Several common sequences emitted by idf_monitor.py will leak through:
- CSI sequences with
?modifier, e.g.\x1b[?25h(cursor show/hide) - OSC sequences:
\x1b]0;title\x07(terminal title) - Single-char escapes:
\x1b(B(character set)
A more comprehensive pattern:
_ANSI_RE = re.compile(
r'\x1b(?:'
r'\[[0-9;?]*[A-Za-z]' # CSI sequences (including ?)
r'|\][^\x07]*(?:\x07|\x1b\\)' # OSC sequences
r'|[()][A-Z]' # Character set designations
r'|[A-Z]' # Single-char Fe escapes
r')'
)|
|
||
| # === Monitor Tools === | ||
|
|
||
| def _require_pty(tool_name: str) -> str | None: |
There was a problem hiding this comment.
_require_pty captures nothing from the enclosing scope — move to module level
This helper is defined inside start_mcp_server but doesn't reference any local variables from that scope. Placing it at module level alongside the other _pty_* helper functions would make the organisation cleaner and easier to test in isolation.
|
Thank you for the contribution. The main issue I see that this is not compatible with Windows which is a must for all of our tools. Also the documentation section about MCP support needs to be updated. |
|
Also please note that the contribution guide states that pre-commit hooks must pass for contributions. See the failed check for more information. |
|
@dobairoland Thanks for the feedback. The current implementation wraps I've been exploring two approaches to fix this and wanted to get your input on which direction to take: Option A: pyserial + address decoding
Option B: PTY on Unix + PyWinpty/ConPTY on Windows
My preference is Option A unless preserving full idf.py monitor parity is a requirement. |
|
@jinw06k I think option B is the best and then we will have to test this if we haven't missed anything Window-related. The issue with option A is that we would start to develop a new monitor implementation which would have to be maintained in addition to esp-idf-monitor. Here, we could prepare a pyserial object and pass it to esp-idf-monitor's internals but the port, baud and similar would also have to be re-implemented. We could also remove the writing capability (and hence the necessity of PTY) but then interactive applications (eg. https://github.com/espressif/esp-idf/tree/44c77cbf46844cd056c923277ece745173cb270d/examples/system/console/advanced ) could not be used. And I think it would be useful if the MCP server would able to control the ESP application that way. So all in all, we need to evaluate option B on Windows with a console-based interactive application. |
|
@dobairoland Thanks for the feedback on Option B. Before I go down the PyWinpty path, I wanted to share a third option (prototyped and tested). Option C: Add --non-interactive mode to
Just around 15 lines of code changes to
I tested this locally, and all IDF monitor features work normally. The MCP server can just set the env var and read stdout. Option C does not require PTY, platform-specific code, or new dependencies. It does, however, require another PR to the esp-idf-monitor repo. Happy to go with Option B if you prefer keeping changes to one repo, but Option C would also benefit in the long run. |
|
Yes, I didn't explain it in too much details but essentially this was option C:
The issue with this that we'd do a lot of hacking and then comes someone who really needs this to be interactive and then we will have to implement option B. |
|
@dobairoland Interactive writing works in my Option C prototype. The My "non-interactive mode" wording may have been misleading — it's more of a headless mode. Full read and write work, just without the |
|
Thank you for the explanation. Option B still sounds the best to me. |
|
Understood. I will update with option B! |
5 participants
Description
Add monitor support to the ESP-IDF MCP server for interactive serial workflows.
This PR introduces PTY-backed monitor tools that allow MCP clients to start an
idf.py monitorsession, send input to the device, read buffered serial output, stop the session, and capture boot logs in a one-shot flow.Changes included
monitor_startmonitor_sendmonitor_readmonitor_stopdevice_reset_and_captureidf.py monitoridf.pycommand construction into_idf_cmd()Motivation
idf.py monitorrequires a TTY-like environment, so it cannot be integrated the same way as the existing build/flash/clean tools. This change adds a PTY-backed monitor session model so MCP clients can interact with device serial output in a more complete way while keeping the existing non-monitor tools unchanged.Related
N/A
Testing
Tested manually on a local MacOS ESP-IDF setup.
Tested flows
monitor_startresets the board and returns initial boot output inlinemonitor_readpolls buffered output with remaining-line indicatormonitor_sendwrites commands to the device; response retrieved viamonitor_readmonitor_stopkills the full process tree and drains remaining outputmonitor_bootcaptures boot log in a one-shot flow with clean cleanupPlatform notes
Checklist
Before submitting a Pull Request, please ensure the following:
Note
Medium Risk
Introduces long-lived subprocess/PTY session management and process-group termination logic, which can affect serial port usage and cleanup behavior across platforms. Non-monitor tools are mostly unchanged but rely on new
_idf_cmd()IDF_PATH validation.Overview
Adds interactive serial monitoring to the MCP server by introducing PTY-backed tools:
monitor_start,monitor_send,monitor_read,monitor_stop, plus a one-shotmonitor_bootthat resets and captures boot logs for a bounded duration.Refactors
idf.pyinvocation into_idf_cmd()(with explicitIDF_PATHvalidation) and adds aMonitorSessionwith background output buffering, ANSI stripping, and robust process-group cleanup; monitor features are guarded on platforms without PTY support and sessions are cleaned up on server exit.Written by Cursor Bugbot for commit 78b3581. This will update automatically on new commits. Configure here.