feat: add embedded MicroPython VM with run_python tool - #128
Open
crispyberry wants to merge 2 commits into
Open
Conversation
Embed MicroPython via the official embed port to give the agent the ability to execute sandboxed Python scripts (calculations, data processing, text manipulation, etc.) through a new `run_python` tool. - Add micropython_embed ESP-IDF component (mpconfigport.h disables network/filesystem/hardware modules for sandbox safety) - Add micropython_vm wrapper: PSRAM heap alloc, mutex, esp_timer timeout with VM hook, stdout capture via mp_hal_stdout_tx_strn - Register run_python tool in tool_registry and context_builder - Add build_micropython_embed.sh helper script - Update skill-creator.md with Python-enabled skills section Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds an embedded MicroPython VM: build scripts and CMake for a micropython_embed component, port headers and HAL stubs, a thread-safe VM harness with timeout and stdout capture, a run_python tool and registry entry, config macros, docs, and a script to generate MicroPython embed artifacts. Changes
Sequence DiagramssequenceDiagram
participant App as Application (app_main)
participant VM as MicroPython VM (micropython_vm.c)
participant Timer as ESP Timer
participant SPIRAM as PSRAM Heap
App->>VM: micropython_vm_init()
VM->>VM: create mutex
VM->>Timer: create timeout timer
VM-->>App: ready
App->>VM: micropython_vm_exec(code, outbuf, timeout)
VM->>VM: acquire mutex
VM->>SPIRAM: allocate GC heap
VM->>Timer: start timer
VM->>VM: init interpreter & execute
VM->>VM: capture stdout -> outbuf
VM->>Timer: stop timer
VM->>SPIRAM: free GC heap
VM->>VM: release mutex
VM-->>App: return result/output
sequenceDiagram
participant Tool as Tool Registry (run_python)
participant Parser as JSON Parser
participant VM as MicroPython VM (micropython_vm_exec)
participant Out as Output Buffer
Tool->>Parser: parse input JSON
Parser-->>Tool: code, timeout
alt invalid input
Tool->>Out: write error
Tool-->>Tool: return ESP_ERR_INVALID_ARG
else valid
Tool->>VM: execute(code, outbuf, timeout)
VM->>Out: capture stdout/exceptions
VM-->>Tool: return esp_err_t
Tool-->>Tool: return result
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (6)
scripts/build_micropython_embed.sh (3)
38-38: Quote command substitution to prevent word splitting.ShellCheck SC2046: The
$(nproc ...)output should be quoted to prevent potential word splitting issues.Proposed fix
-make -C "${MP_DIR}/mpy-cross" -j$(nproc 2>/dev/null || sysctl -n hw.ncpu 2>/dev/null || echo 2) +make -C "${MP_DIR}/mpy-cross" -j"$(nproc 2>/dev/null || sysctl -n hw.ncpu 2>/dev/null || echo 2)"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/build_micropython_embed.sh` at line 38, The make invocation uses unquoted command substitution for the -j argument which can cause word-splitting; update the make command that calls make -C "${MP_DIR}/mpy-cross" -j$(nproc 2>/dev/null || sysctl -n hw.ncpu 2>/dev/null || echo 2) to quote the substitution (i.e., wrap the entire $(...) in double quotes) so the output of the command substitution is treated as a single token; modify the line containing that make call accordingly to prevent ShellCheck SC2046 warnings.
48-48: Quote command substitution to prevent word splitting.Same issue as line 38 — quote the command substitution for consistency and safety.
Proposed fix
-make -C "${MP_DIR}/ports/embed" -j$(nproc 2>/dev/null || sysctl -n hw.ncpu 2>/dev/null || echo 2) +make -C "${MP_DIR}/ports/embed" -j"$(nproc 2>/dev/null || sysctl -n hw.ncpu 2>/dev/null || echo 2)"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/build_micropython_embed.sh` at line 48, The make invocation uses an unquoted command substitution for the -j argument which can cause word-splitting; update the make command in the embed build step (the line invoking make -C "${MP_DIR}/ports/embed" -j$(nproc 2>/dev/null || sysctl -n hw.ncpu 2>/dev/null || echo 2)) to quote the command substitution so the entire CPU-count string is treated as a single argument (e.g., -j"$(nproc ... || ... || echo 2)"), preserving the existing redirections and fallbacks.
30-31: Consider explicit submodule existence check.The
2>/dev/null || truepattern swallows all errors fromgit submodule add, making it harder to diagnose genuine failures. Consider checking if the submodule entry already exists:Proposed alternative
- git -C "${PROJECT_DIR}" submodule add https://github.com/micropython/micropython.git lib/micropython 2>/dev/null || true - git -C "${PROJECT_DIR}" submodule update --init lib/micropython + if ! git -C "${PROJECT_DIR}" config --file .gitmodules --get submodule.lib/micropython.path >/dev/null 2>&1; then + git -C "${PROJECT_DIR}" submodule add https://github.com/micropython/micropython.git lib/micropython + fi + git -C "${PROJECT_DIR}" submodule update --init lib/micropython🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/build_micropython_embed.sh` around lines 30 - 31, The script currently silences failures from `git -C "${PROJECT_DIR}" submodule add ... lib/micropython` using `2>/dev/null || true`; instead detect whether the submodule is already configured (e.g. check for a 'path = lib/micropython' entry in "${PROJECT_DIR}/.gitmodules" or test existence of "${PROJECT_DIR}/lib/micropython/.git") and only run `git submodule add` when missing, removing the `2>/dev/null || true` suppression so legitimate errors surface; keep the subsequent `git -C "${PROJECT_DIR}" submodule update --init lib/micropython` as-is.main/agent/context_builder.c (1)
49-51: Consider includingheapqin the available modules list.The system prompt lists available modules, but
heapqis enabled inmpconfigport.h(line 46:MICROPY_PY_HEAPQ (1)) but not mentioned here. For consistency, consider adding it to the list or disabling it in the config if it's not intended to be available.Proposed fix
- "- run_python: Execute Python code via embedded MicroPython VM. Use print() for output. " - "Available modules: math, json, re, collections, struct, binascii, random. No network/file/hardware access.\n\n" + "- run_python: Execute Python code via embedded MicroPython VM. Use print() for output. " + "Available modules: math, json, re, collections, heapq, struct, binascii, random. No network/file/hardware access.\n\n"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@main/agent/context_builder.c` around lines 49 - 51, The run_python description string in context_builder.c currently lists available MicroPython modules but omits heapq; update that string (the literal that starts with "- run_python: Execute Python code..." and lists "math, json, re, collections, struct, binascii, random") to include "heapq" so the prompt matches mpconfigport.h's MICROPY_PY_HEAPQ setting, or alternatively disable heapq in the MicroPython config if it should not be exposed.main/tools/tool_registry.c (1)
186-190: Make the schema match the runtime timeout contract.The executor clamps
timeout_msto100..30000and defaults toMIMI_MICROPYTHON_TIMEOUT_MS, but the schema only saysintegerand hard-codes10000in the description. That metadata will drift as soon as the config changes. Addminimum/maximum, and avoid baking the default into the string unless it is generated from the macro.📐 Suggested schema update
.input_schema_json = "{\"type\":\"object\"," "\"properties\":{\"code\":{\"type\":\"string\",\"description\":\"Python code to execute. Use print() to produce output.\"}," - "\"timeout_ms\":{\"type\":\"integer\",\"description\":\"Execution timeout in milliseconds (default 10000, max 30000)\"}}," + "\"timeout_ms\":{\"type\":\"integer\",\"minimum\":100,\"maximum\":30000," + "\"description\":\"Execution timeout in milliseconds\"}}," "\"required\":[\"code\"]}",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@main/tools/tool_registry.c` around lines 186 - 190, The input_schema_json for the MicroPython executor (.input_schema_json in tool_registry.c) must match the runtime contract: add "minimum":100 and "maximum":30000 to the timeout_ms property and remove the hard-coded "10000" default from the description; either reference the actual default macro MIMI_MICROPYTHON_TIMEOUT_MS or omit the default text so it won't drift when the macro changes. Update the timeout_ms property description accordingly and keep the rest of the schema intact.main/mimi_config.h (1)
126-126:MIMI_MICROPYTHON_OUTPUT_SIZEis not wired into the execution path.
tool_run_python_execute()forwards the caller'soutput_sizeunchanged tomicropython_vm_exec(), and existing callers already pass different buffer sizes. Changing this macro here will not actually change the VM's output limit, so the config knob is misleading unless the callers are updated to use it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@main/mimi_config.h` at line 126, The macro MIMI_MICROPYTHON_OUTPUT_SIZE is defined but never used in the execution path; tool_run_python_execute currently forwards the caller-provided output_size to micropython_vm_exec, so changing the macro has no effect. Fix by wiring the macro into the call chain: update tool_run_python_execute to default/replace the forwarded output_size with MIMI_MICROPYTHON_OUTPUT_SIZE (or clamp the caller value to it) before calling micropython_vm_exec, or alternatively update the existing callers to pass MIMI_MICROPYTHON_OUTPUT_SIZE when they want the configured limit; reference symbols: MIMI_MICROPYTHON_OUTPUT_SIZE, tool_run_python_execute, micropython_vm_exec.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@main/CMakeLists.txt`:
- Around line 28-31: Move the build dependency for micropython_embed from the
public REQUIRES list to PRIV_REQUIRES in the main CMakeLists.txt so it is only
linked privately for the main target; update the REQUIRES/PRIV_REQUIRES entries
so micropython_embed is removed from REQUIRES and added to PRIV_REQUIRES
(micropython_vm.c remains able to use it while micropython_vm.h stays
dependency-free of MicroPython types).
In `@main/micropython/micropython_vm.c`:
- Around line 29-40: mp_hal_stdout_tx_strn currently drops data silently when
the buffer is full (using s_stdout_buf, s_stdout_size, s_stdout_pos); fix it by
adding a truncation flag (e.g. s_stdout_truncated) that is set whenever copy <
len and by attempting to append a visible marker like "…(truncated)" if there is
any remaining space (respecting s_stdout_size and null-terminator), ensuring
s_stdout_pos and termination are updated correctly; also add a helper to
reset/check s_stdout_truncated so callers can detect clipped output.
- Line 11: The build fails because micropython_vm.c includes micropython_embed.h
which is not produced or not on the compiler include path; update the build so
the header is generated and/or added to the compiler's include directories
before compiling micropython_vm.c. Modify the build rules that produce
micropython_embed.h (or add a dependency) so its generation runs prior to
compiling the target that builds micropython_vm.c, and add the directory
containing micropython_embed.h to the compiler include paths (e.g., via the
project's CMakeLists/Makefile) so the compiler can locate micropython_embed.h
during the build.
- Around line 92-125: The function currently ignores Python execution failures
because ret is never updated after calling mp_embed_exec_str; modify the code so
mp_embed_exec_str returns a status (e.g., esp_err_t or int) indicating success,
exception, or timeout, then assign that to ret (and/or set ret = ESP_ERR_TIMEOUT
when micropython_vm_timeout_flag is set) before stopping the timer and
returning; update callers of mp_embed_exec_str/mp_embed_exec_str signature and
ensure symbols mentioned (ret, mp_embed_exec_str, micropython_vm_timeout_flag,
esp_timer_stop) are used to propagate non-ESP_OK results upward.
- Around line 51-54: The timeout currently sets micropython_vm_timeout_flag in
timeout_callback but relies on mp_sched_keyboard_interrupt (which raises a
catchable KeyboardInterrupt) so user code can swallow it; instead either latch
the timeout until VM teardown or abort non-catchably. Implement one of: (A)
change timeout_callback to set a persistent latched flag (e.g., add
micropython_vm_timeout_latched) and ensure the scheduler loop that calls
mp_sched_keyboard_interrupt checks micropython_vm_timeout_latched and, once set,
forces teardown/stop without clearing the latch; or (B) have timeout_callback
trigger a non-catchable abort path by calling nlr_jump_fail()/process
termination directly instead of setting micropython_vm_timeout_flag so execution
cannot continue. Update uses of
micropython_vm_timeout_flag/mp_sched_keyboard_interrupt accordingly to respect
the chosen approach.
In `@main/tools/tool_run_python.c`:
- Around line 11-23: The function tool_run_python_execute writes into the
caller-provided output buffer before validating it; add an early guard at the
top of tool_run_python_execute to validate output and output_size (e.g., require
output != NULL and output_size > 0 per the micropython_vm_exec contract) and
return ESP_ERR_INVALID_ARG without touching output or parsing JSON if the guard
fails; ensure all existing error paths (including after cJSON_Parse failures and
code_obj checks) no longer assume output is non-NULL and keep cJSON_Delete(root)
where appropriate so there are no leaks.
---
Nitpick comments:
In `@main/agent/context_builder.c`:
- Around line 49-51: The run_python description string in context_builder.c
currently lists available MicroPython modules but omits heapq; update that
string (the literal that starts with "- run_python: Execute Python code..." and
lists "math, json, re, collections, struct, binascii, random") to include
"heapq" so the prompt matches mpconfigport.h's MICROPY_PY_HEAPQ setting, or
alternatively disable heapq in the MicroPython config if it should not be
exposed.
In `@main/mimi_config.h`:
- Line 126: The macro MIMI_MICROPYTHON_OUTPUT_SIZE is defined but never used in
the execution path; tool_run_python_execute currently forwards the
caller-provided output_size to micropython_vm_exec, so changing the macro has no
effect. Fix by wiring the macro into the call chain: update
tool_run_python_execute to default/replace the forwarded output_size with
MIMI_MICROPYTHON_OUTPUT_SIZE (or clamp the caller value to it) before calling
micropython_vm_exec, or alternatively update the existing callers to pass
MIMI_MICROPYTHON_OUTPUT_SIZE when they want the configured limit; reference
symbols: MIMI_MICROPYTHON_OUTPUT_SIZE, tool_run_python_execute,
micropython_vm_exec.
In `@main/tools/tool_registry.c`:
- Around line 186-190: The input_schema_json for the MicroPython executor
(.input_schema_json in tool_registry.c) must match the runtime contract: add
"minimum":100 and "maximum":30000 to the timeout_ms property and remove the
hard-coded "10000" default from the description; either reference the actual
default macro MIMI_MICROPYTHON_TIMEOUT_MS or omit the default text so it won't
drift when the macro changes. Update the timeout_ms property description
accordingly and keep the rest of the schema intact.
In `@scripts/build_micropython_embed.sh`:
- Line 38: The make invocation uses unquoted command substitution for the -j
argument which can cause word-splitting; update the make command that calls make
-C "${MP_DIR}/mpy-cross" -j$(nproc 2>/dev/null || sysctl -n hw.ncpu 2>/dev/null
|| echo 2) to quote the substitution (i.e., wrap the entire $(...) in double
quotes) so the output of the command substitution is treated as a single token;
modify the line containing that make call accordingly to prevent ShellCheck
SC2046 warnings.
- Line 48: The make invocation uses an unquoted command substitution for the -j
argument which can cause word-splitting; update the make command in the embed
build step (the line invoking make -C "${MP_DIR}/ports/embed" -j$(nproc
2>/dev/null || sysctl -n hw.ncpu 2>/dev/null || echo 2)) to quote the command
substitution so the entire CPU-count string is treated as a single argument
(e.g., -j"$(nproc ... || ... || echo 2)"), preserving the existing redirections
and fallbacks.
- Around line 30-31: The script currently silences failures from `git -C
"${PROJECT_DIR}" submodule add ... lib/micropython` using `2>/dev/null || true`;
instead detect whether the submodule is already configured (e.g. check for a
'path = lib/micropython' entry in "${PROJECT_DIR}/.gitmodules" or test existence
of "${PROJECT_DIR}/lib/micropython/.git") and only run `git submodule add` when
missing, removing the `2>/dev/null || true` suppression so legitimate errors
surface; keep the subsequent `git -C "${PROJECT_DIR}" submodule update --init
lib/micropython` as-is.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 44705e87-7757-46f6-b73f-e8f3c92688e5
⛔ Files ignored due to path filters (1)
components/micropython_embed/generated/.gitkeepis excluded by!**/generated/**
📒 Files selected for processing (15)
.gitignorecomponents/micropython_embed/CMakeLists.txtcomponents/micropython_embed/port/mpconfigport.hcomponents/micropython_embed/port/mphalport.hmain/CMakeLists.txtmain/agent/context_builder.cmain/micropython/micropython_vm.cmain/micropython/micropython_vm.hmain/mimi.cmain/mimi_config.hmain/tools/tool_registry.cmain/tools/tool_run_python.cmain/tools/tool_run_python.hscripts/build_micropython_embed.shspiffs_data/skills/skill-creator.md
Comment on lines
28
to
+31
| REQUIRES | ||
| nvs_flash esp_wifi esp_netif esp_http_client esp_http_server | ||
| esp_https_ota esp_event json spiffs console vfs app_update esp-tls | ||
| esp_timer esp_websocket_client | ||
| esp_timer esp_websocket_client micropython_embed |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if MicroPython types are exposed in public headers
rg -l 'mp_int_t|mp_uint_t|mp_off_t|micropython' --type=c --type-add 'c:*.h' -g '!micropython*' main/Repository: memovai/mimiclaw
Length of output: 100
🏁 Script executed:
# Find all header files in main/ and check which contain MicroPython references
find main/ -name "*.h" -type f | head -20Repository: memovai/mimiclaw
Length of output: 581
🏁 Script executed:
# Check the full CMakeLists.txt to understand public header configuration
cat main/CMakeLists.txtRepository: memovai/mimiclaw
Length of output: 1054
🏁 Script executed:
# Check if there are any includes of micropython headers in the public headers
rg 'include.*micropython|mp_int_t|mp_uint_t|mp_obj' main/ -t c --type-add 'c:*.h' | head -30Repository: memovai/mimiclaw
Length of output: 469
🏁 Script executed:
cat main/micropython/micropython_vm.hRepository: memovai/mimiclaw
Length of output: 871
🏁 Script executed:
# Also check if other headers in main/ that are likely public include micropython_vm.h
rg '#include.*micropython_vm' main/ -t c --type-add 'c:*.h'Repository: memovai/mimiclaw
Length of output: 242
Move micropython_embed from REQUIRES to PRIV_REQUIRES.
The public header micropython_vm.h does not expose any MicroPython types—it only uses standard types like esp_err_t and size_t. Since micropython_embed is only needed for internal implementation in micropython_vm.c, it should be a private dependency to avoid unnecessarily exporting it to all components that depend on main.
Proposed fix
REQUIRES
nvs_flash esp_wifi esp_netif esp_http_client esp_http_server
esp_https_ota esp_event json spiffs console vfs app_update esp-tls
- esp_timer esp_websocket_client micropython_embed
+ esp_timer esp_websocket_client
+ PRIV_REQUIRES
+ micropython_embed
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| REQUIRES | |
| nvs_flash esp_wifi esp_netif esp_http_client esp_http_server | |
| esp_https_ota esp_event json spiffs console vfs app_update esp-tls | |
| esp_timer esp_websocket_client | |
| esp_timer esp_websocket_client micropython_embed | |
| REQUIRES | |
| nvs_flash esp_wifi esp_netif esp_http_client esp_http_server | |
| esp_https_ota esp_event json spiffs console vfs app_update esp-tls | |
| esp_timer esp_websocket_client | |
| PRIV_REQUIRES | |
| micropython_embed | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@main/CMakeLists.txt` around lines 28 - 31, Move the build dependency for
micropython_embed from the public REQUIRES list to PRIV_REQUIRES in the main
CMakeLists.txt so it is only linked privately for the main target; update the
REQUIRES/PRIV_REQUIRES entries so micropython_embed is removed from REQUIRES and
added to PRIV_REQUIRES (micropython_vm.c remains able to use it while
micropython_vm.h stays dependency-free of MicroPython types).
| #include "esp_log.h" | ||
| #include "esp_heap_caps.h" | ||
|
|
||
| #include "micropython_embed.h" |
There was a problem hiding this comment.
Wire micropython_embed.h into the build before merge.
CI is already failing on this include with micropython_embed.h: No such file or directory. In its current state, a clean checkout cannot compile this translation unit, so the feature still depends on a manual pre-generation step that the build is not performing.
🧰 Tools
🪛 GitHub Actions: Build
[error] 11-11: micropython_embed.h: No such file or directory
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@main/micropython/micropython_vm.c` at line 11, The build fails because
micropython_vm.c includes micropython_embed.h which is not produced or not on
the compiler include path; update the build so the header is generated and/or
added to the compiler's include directories before compiling micropython_vm.c.
Modify the build rules that produce micropython_embed.h (or add a dependency) so
its generation runs prior to compiling the target that builds micropython_vm.c,
and add the directory containing micropython_embed.h to the compiler include
paths (e.g., via the project's CMakeLists/Makefile) so the compiler can locate
micropython_embed.h during the build.
Comment on lines
+29
to
+40
| void mp_hal_stdout_tx_strn(const char *str, mp_uint_t len) | ||
| { | ||
| if (!s_stdout_buf) return; | ||
| size_t avail = (s_stdout_size > s_stdout_pos + 1) | ||
| ? s_stdout_size - s_stdout_pos - 1 | ||
| : 0; | ||
| size_t copy = (len < avail) ? len : avail; | ||
| if (copy > 0) { | ||
| memcpy(s_stdout_buf + s_stdout_pos, str, copy); | ||
| s_stdout_pos += copy; | ||
| s_stdout_buf[s_stdout_pos] = '\0'; | ||
| } |
There was a problem hiding this comment.
Don't silently drop stdout after the buffer fills.
Once avail reaches zero, the rest of the script output is discarded and the call still succeeds. That can turn JSON, CSV, or other structured results into partial data with no signal to the caller. Track truncation and append a marker or return an error when output is clipped.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@main/micropython/micropython_vm.c` around lines 29 - 40,
mp_hal_stdout_tx_strn currently drops data silently when the buffer is full
(using s_stdout_buf, s_stdout_size, s_stdout_pos); fix it by adding a truncation
flag (e.g. s_stdout_truncated) that is set whenever copy < len and by attempting
to append a visible marker like "…(truncated)" if there is any remaining space
(respecting s_stdout_size and null-terminator), ensuring s_stdout_pos and
termination are updated correctly; also add a helper to reset/check
s_stdout_truncated so callers can detect clipped output.
Comment on lines
+51
to
+54
| static void timeout_callback(void *arg) | ||
| { | ||
| micropython_vm_timeout_flag = 1; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In MicroPython, does mp_sched_keyboard_interrupt()raise a normal catchableKeyboardInterrupt, and can execution continue after user code catches it?
💡 Result:
Yes.
mp_sched_keyboard_interrupt()doesn’t “hard abort” the VM; it arranges for aKeyboardInterruptexception object to be raised the next time the VM checks pending work (“as soon as the VM resumes”). (sources.debian.org)- Internally, the keyboard-interrupt path sets
mp_pending_exceptionto the preallocated keyboard-exception object and the VM later delivers it by callingnlr_raise(obj)—i.e. it’s raised like a normal Python exception. (blog.weghos.com) - Because it’s delivered as a normal exception, user code can catch
KeyboardInterruptwithtry/except KeyboardInterrupt:and execution can continue after theexceptblock (the pending-exception slot is cleared before raising, so it won’t immediately re-trigger unless another interrupt is scheduled). (blog.weghos.com)
(Separately, at the Python level, MicroPython documents Ctrl-C as raising KeyboardInterrupt.) (micropython.org)
Citations:
- 1: https://sources.debian.org/src/micropython/1.25.0%2Bds-1/extmod/os_dupterm.c
- 2: https://blog.weghos.com/micropython/MicroPython/py/scheduler.c.html
- 3: https://blog.weghos.com/micropython/MicroPython/py/scheduler.c.html
- 4: https://micropython.org/resources/docs/en/latest/library/micropython.html?utm_source=openai
The timeout is catchable and not enforced.
mp_sched_keyboard_interrupt() raises a standard KeyboardInterrupt that user code can catch with try/except and continue execution. The pending-exception flag clears before raising, so it won't re-trigger unless another interrupt is scheduled. This is not a hard execution limit. Either latch the timeout across multiple scheduling cycles until teardown, or transition to a non-catchable abort path (e.g., nlr_jump_fail() or process termination).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@main/micropython/micropython_vm.c` around lines 51 - 54, The timeout
currently sets micropython_vm_timeout_flag in timeout_callback but relies on
mp_sched_keyboard_interrupt (which raises a catchable KeyboardInterrupt) so user
code can swallow it; instead either latch the timeout until VM teardown or abort
non-catchably. Implement one of: (A) change timeout_callback to set a persistent
latched flag (e.g., add micropython_vm_timeout_latched) and ensure the scheduler
loop that calls mp_sched_keyboard_interrupt checks
micropython_vm_timeout_latched and, once set, forces teardown/stop without
clearing the latch; or (B) have timeout_callback trigger a non-catchable abort
path by calling nlr_jump_fail()/process termination directly instead of setting
micropython_vm_timeout_flag so execution cannot continue. Update uses of
micropython_vm_timeout_flag/mp_sched_keyboard_interrupt accordingly to respect
the chosen approach.
Comment on lines
+92
to
+125
| esp_err_t ret = ESP_OK; | ||
|
|
||
| /* Allocate GC heap from PSRAM */ | ||
| void *gc_heap = heap_caps_calloc(1, MIMI_MICROPYTHON_HEAP_SIZE, MALLOC_CAP_SPIRAM); | ||
| if (!gc_heap) { | ||
| snprintf(output, output_size, "Error: failed to allocate MicroPython heap (%dKB)", | ||
| MIMI_MICROPYTHON_HEAP_SIZE / 1024); | ||
| ret = ESP_ERR_NO_MEM; | ||
| goto unlock; | ||
| } | ||
|
|
||
| /* Setup stdout capture */ | ||
| s_stdout_buf = output; | ||
| s_stdout_pos = 0; | ||
| s_stdout_size = output_size; | ||
| output[0] = '\0'; | ||
|
|
||
| /* Reset timeout flag */ | ||
| micropython_vm_timeout_flag = 0; | ||
|
|
||
| /* Init interpreter */ | ||
| mp_embed_init(gc_heap, MIMI_MICROPYTHON_HEAP_SIZE); | ||
|
|
||
| /* Start timeout timer */ | ||
| if (timeout_ms > 0) { | ||
| esp_timer_start_once(s_timeout_timer, (uint64_t)timeout_ms * 1000); | ||
| } | ||
|
|
||
| /* Execute — exceptions are printed to stdout via mp_hal_stdout_tx_strn */ | ||
| mp_embed_exec_str(code); | ||
|
|
||
| /* Cancel timeout */ | ||
| esp_timer_stop(s_timeout_timer); | ||
| micropython_vm_timeout_flag = 0; |
There was a problem hiding this comment.
Propagate script failures via esp_err_t.
ret never changes after mp_embed_exec_str(code), so uncaught Python exceptions and timeout-triggered failures only show up as printed text in output. Callers above this layer treat non-ESP_OK as failure, which means tracebacks/timeouts currently bubble up as successful tool results.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@main/micropython/micropython_vm.c` around lines 92 - 125, The function
currently ignores Python execution failures because ret is never updated after
calling mp_embed_exec_str; modify the code so mp_embed_exec_str returns a status
(e.g., esp_err_t or int) indicating success, exception, or timeout, then assign
that to ret (and/or set ret = ESP_ERR_TIMEOUT when micropython_vm_timeout_flag
is set) before stopping the timer and returning; update callers of
mp_embed_exec_str/mp_embed_exec_str signature and ensure symbols mentioned (ret,
mp_embed_exec_str, micropython_vm_timeout_flag, esp_timer_stop) are used to
propagate non-ESP_OK results upward.
Comment on lines
+11
to
+23
| esp_err_t tool_run_python_execute(const char *input_json, char *output, size_t output_size) | ||
| { | ||
| cJSON *root = cJSON_Parse(input_json); | ||
| if (!root) { | ||
| snprintf(output, output_size, "Error: invalid JSON input"); | ||
| return ESP_ERR_INVALID_ARG; | ||
| } | ||
|
|
||
| cJSON *code_obj = cJSON_GetObjectItem(root, "code"); | ||
| if (!code_obj || !cJSON_IsString(code_obj)) { | ||
| cJSON_Delete(root); | ||
| snprintf(output, output_size, "Error: 'code' field is required and must be a string"); | ||
| return ESP_ERR_INVALID_ARG; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In C, is it valid to call snprintf with a NULL destination pointer when the size argument is greater than zero?
💡 Result:
No. In standard C, snprintf may be called with a null destination pointer only when the size argument is 0. If the size argument is greater than 0, the destination pointer must point to a writable array; passing NULL is undefined behavior. [1][2]
So: snprintf(NULL, 0, ...) is valid (commonly used to compute the required length), but snprintf(NULL, n>0, ...) is not. [1][2]
Sources: [1] (cppreference) [2] (glibc manual)
🏁 Script executed:
cd main/tools && wc -l tool_run_python.cRepository: memovai/mimiclaw
Length of output: 80
🏁 Script executed:
cat -n main/tools/tool_run_python.c | head -50Repository: memovai/mimiclaw
Length of output: 1925
Validate the caller's output buffer before the first error path.
The parse/validation branches write to output before checking it. If a caller passes output == NULL with output_size > 0, this causes undefined behavior instead of returning ESP_ERR_INVALID_ARG; and output_size == 0 currently disagrees with the lower-level micropython_vm_exec() contract.
🛡️ Proposed guard
esp_err_t tool_run_python_execute(const char *input_json, char *output, size_t output_size)
{
+ if (!output || output_size == 0) {
+ return ESP_ERR_INVALID_ARG;
+ }
+ if (!input_json) {
+ snprintf(output, output_size, "Error: input JSON is required");
+ return ESP_ERR_INVALID_ARG;
+ }
+
cJSON *root = cJSON_Parse(input_json);
if (!root) {
snprintf(output, output_size, "Error: invalid JSON input");
return ESP_ERR_INVALID_ARG;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@main/tools/tool_run_python.c` around lines 11 - 23, The function
tool_run_python_execute writes into the caller-provided output buffer before
validating it; add an early guard at the top of tool_run_python_execute to
validate output and output_size (e.g., require output != NULL and output_size >
0 per the micropython_vm_exec contract) and return ESP_ERR_INVALID_ARG without
touching output or parsing JSON if the guard fails; ensure all existing error
paths (including after cJSON_Parse failures and code_obj checks) no longer
assume output is non-NULL and keep cJSON_Delete(root) where appropriate so there
are no leaks.
- Update CMakeLists to glob all generated .c files (embed port produces a directory tree, not a single amalgamated file) - Add MICROPY_GCREGS_SETJMP and MICROPY_NLR_SETJMP to mpconfigport.h to fix GC register scanning and linker relocation errors on Xtensa - Update micropython_vm.c for new embed API (mp_embed_init takes stack_top, mp_hal_stdout_tx_strn returns mp_uint_t) - Add stubs for mp_import_stat, mp_lexer_new_from_file, mp_builtin_open - Update .gitignore to exclude entire generated/ directory - Document MicroPython VM and run_python tool in README Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
components/micropython_embed/port/mpconfigport.h (1)
76-82: Drop the trailing;from thedo { } while (0)macro.Standard idiom is to omit the terminator so the macro can be used as a statement without creating a double-semicolon / null-statement, and so it remains safe inside braceless
if/else. Upstream MicroPython invokes these hooks asMICROPY_VM_HOOK_LOOP— the call site supplies its own;.♻️ Proposed nit
`#define` MICROPY_VM_HOOK_LOOP \ do { \ if (micropython_vm_timeout_flag) { \ micropython_vm_timeout_flag = 0; \ mp_sched_keyboard_interrupt(); \ } \ - } while (0); + } while (0)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/micropython_embed/port/mpconfigport.h` around lines 76 - 82, The macro MICROPY_VM_HOOK_LOOP currently ends with an extraneous semicolon which causes a double-semicolon/null-statement at call sites and can break usage in braceless if/else; edit the macro definition in mpconfigport.h to remove the trailing ';' after the do { ... } while (0) so the hook can be invoked as MICROPY_VM_HOOK_LOOP by callers; leave the body unchanged (it should still check micropython_vm_timeout_flag, reset it, and call mp_sched_keyboard_interrupt()) but ensure the macro terminator is just the while(0) without a final semicolon.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@main/micropython/micropython_vm.c`:
- Around line 143-170: Gate the call to esp_timer_stop(s_timeout_timer) so it
only runs when the timer was actually started (i.e., when timeout_ms > 0 or an
armed flag) to avoid ESP_ERR_INVALID_STATE warnings; update the teardown in the
same block that started the timer (or track an 'armed' bool) before calling
esp_timer_stop. Also, after writing the fallback message with snprintf(output,
output_size, "(no output — use print() to see results)"), update s_stdout_pos to
reflect the new output length (e.g., strlen(output) or the snprintf return) so
the subsequent ESP_LOGI(TAG, "Python exec done (%d bytes output)",
(int)s_stdout_pos) logs the correct byte count.
---
Nitpick comments:
In `@components/micropython_embed/port/mpconfigport.h`:
- Around line 76-82: The macro MICROPY_VM_HOOK_LOOP currently ends with an
extraneous semicolon which causes a double-semicolon/null-statement at call
sites and can break usage in braceless if/else; edit the macro definition in
mpconfigport.h to remove the trailing ';' after the do { ... } while (0) so the
hook can be invoked as MICROPY_VM_HOOK_LOOP by callers; leave the body unchanged
(it should still check micropython_vm_timeout_flag, reset it, and call
mp_sched_keyboard_interrupt()) but ensure the macro terminator is just the
while(0) without a final semicolon.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 24c39228-9abd-4d81-9bcf-b6fa0e477694
📒 Files selected for processing (5)
.gitignoreREADME.mdcomponents/micropython_embed/CMakeLists.txtcomponents/micropython_embed/port/mpconfigport.hmain/micropython/micropython_vm.c
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (1)
- components/micropython_embed/CMakeLists.txt
Comment on lines
+143
to
+170
| /* Start timeout timer */ | ||
| if (timeout_ms > 0) { | ||
| esp_timer_start_once(s_timeout_timer, (uint64_t)timeout_ms * 1000); | ||
| } | ||
|
|
||
| /* Execute — exceptions are printed to stdout via mp_hal_stdout_tx_strn */ | ||
| mp_embed_exec_str(code); | ||
|
|
||
| /* Cancel timeout */ | ||
| esp_timer_stop(s_timeout_timer); | ||
| micropython_vm_timeout_flag = 0; | ||
|
|
||
| /* Teardown interpreter */ | ||
| mp_embed_deinit(); | ||
|
|
||
| /* Free heap */ | ||
| heap_caps_free(gc_heap); | ||
|
|
||
| /* Detach stdout capture */ | ||
| s_stdout_buf = NULL; | ||
| s_stdout_size = 0; | ||
|
|
||
| /* If no output was produced, say so */ | ||
| if (s_stdout_pos == 0) { | ||
| snprintf(output, output_size, "(no output — use print() to see results)"); | ||
| } | ||
|
|
||
| ESP_LOGI(TAG, "Python exec done (%d bytes output)", (int)s_stdout_pos); |
There was a problem hiding this comment.
Two minor cleanups in the exec teardown path.
- Line 152:
esp_timer_stopis called unconditionally, but the timer is only started whentimeout_ms > 0(line 144). Stopping a non-running timer returnsESP_ERR_INVALID_STATEand emits an esp_timer warning. Gate the stop on the same condition (or track anarmedbool). - Lines 166-170: when
s_stdout_pos == 0the fallback message is written viasnprintf, buts_stdout_posis not updated, so the subsequentESP_LOGI("%d bytes output", s_stdout_pos)reports0even thoughoutputis non-empty. Minor log inconsistency.
♻️ Proposed adjustments
/* Start timeout timer */
- if (timeout_ms > 0) {
+ bool timer_armed = (timeout_ms > 0);
+ if (timer_armed) {
esp_timer_start_once(s_timeout_timer, (uint64_t)timeout_ms * 1000);
}
/* Execute — exceptions are printed to stdout via mp_hal_stdout_tx_strn */
mp_embed_exec_str(code);
/* Cancel timeout */
- esp_timer_stop(s_timeout_timer);
+ if (timer_armed) {
+ esp_timer_stop(s_timeout_timer);
+ }
micropython_vm_timeout_flag = 0;
@@
/* If no output was produced, say so */
if (s_stdout_pos == 0) {
- snprintf(output, output_size, "(no output — use print() to see results)");
+ int n = snprintf(output, output_size, "(no output — use print() to see results)");
+ if (n > 0) s_stdout_pos = (size_t)n < output_size ? (size_t)n : output_size - 1;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@main/micropython/micropython_vm.c` around lines 143 - 170, Gate the call to
esp_timer_stop(s_timeout_timer) so it only runs when the timer was actually
started (i.e., when timeout_ms > 0 or an armed flag) to avoid
ESP_ERR_INVALID_STATE warnings; update the teardown in the same block that
started the timer (or track an 'armed' bool) before calling esp_timer_stop.
Also, after writing the fallback message with snprintf(output, output_size, "(no
output — use print() to see results)"), update s_stdout_pos to reflect the new
output length (e.g., strlen(output) or the snprintf return) so the subsequent
ESP_LOGI(TAG, "Python exec done (%d bytes output)", (int)s_stdout_pos) logs the
correct byte count.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Embed MicroPython via the official embed port to give the agent the ability to execute sandboxed Python scripts (calculations, data processing, text manipulation, etc.) through a new
run_pythontool.Summary by CodeRabbit
New Features
Documentation
Chores