Restore plain text tracebacks and fix exit codes for code mode#9224
Restore plain text tracebacks and fix exit codes for code mode#9224
Conversation
Fixes a regression from #8376 brought back HTML-wrapping for errors in code mode. Additionally, errors in child cells now also cause the done event to report `success: false`, so `execute-code.sh` callers get exit code 1. Finally, simplifies `_print_summary` to just report `(error)` in the for cells since it appears in the traceback already.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
for more information, see https://pre-commit.ci
| exc_type = ( | ||
| getattr(err, "exception_type", None) or type(err).__name__ | ||
| ) | ||
| short_id = str(msg.cell_id)[:8] |
There was a problem hiding this comment.
i think this is always short? maybe could lead to a bug otherwise?
There was a problem hiding this comment.
yeah i changed to just the msg.cell_id, so the model gets the full context.
There was a problem hiding this comment.
Pull request overview
This PR fixes code-mode scratchpad error reporting regressions by ensuring tracebacks are plain text (not HTML-highlighted) in code mode and by propagating errors from child cells into the final “done” result so callers can rely on non-zero exit status.
Changes:
- Track and surface errors from non-scratch “child” cells during scratchpad execution, causing the done event / extract_result to report
success: false. - Restore plain-text traceback payloads in code mode (avoid HTML-wrapping/highlighting).
- Simplify code-mode
_print_summaryto avoid duplicating error details that are already present in streamed tracebacks (only annotate with(error)).
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
marimo/_server/scratchpad.py |
Adds child-cell error tracking and uses it in build_done_event / extract_result. |
marimo/_server/api/endpoints/execution.py |
Passes the scratchpad listener into build_done_event so child errors affect success. |
marimo/_mcp/code_server/main.py |
Passes the listener into extract_result so MCP callers see failure on child errors. |
marimo/_messaging/tracebacks.py |
Sends plain text traceback data in code mode, HTML-highlighted otherwise. |
marimo/_code_mode/_context.py |
Updates summary output to append (error) and stops printing duplicated error blocks to stderr. |
tests/_server/test_scratchpad.py |
Adds tests for child-error propagation into done/extract_result behavior. |
tests/_code_mode/test_context.py |
Updates expectations so _print_summary no longer writes runtime errors to stderr. |
|
|
||
| # Add a blank line before the summary when cells errored, | ||
| # so the summary is visually separated from streamed tracebacks. | ||
| has_errors = _run and any(self._cell_errored(cid) for cid in _run) |
There was a problem hiding this comment.
has_errors = _run and any(...) can evaluate to the (possibly empty) set _run rather than a boolean, which is a bit error-prone and can confuse type checkers/readability. Consider making this explicitly boolean (e.g., has_errors = any(self._cell_errored(cid) for cid in _run)).
| has_errors = _run and any(self._cell_errored(cid) for cid in _run) | |
| has_errors = any(self._cell_errored(cid) for cid in _run) |
| if ( | ||
| msg.output is not None | ||
| and msg.output.channel == CellChannel.MARIMO_ERROR | ||
| and isinstance(msg.output.data, list) | ||
| and msg.output.data | ||
| ): | ||
| err = msg.output.data[0] | ||
| exc_type = ( | ||
| getattr(err, "exception_type", None) or type(err).__name__ | ||
| ) | ||
| short_id = str(msg.cell_id)[:8] | ||
| self.child_error_summaries.append( | ||
| f"cell '{short_id}' raised {exc_type}" |
There was a problem hiding this comment.
The new child_error_summaries population logic in on_notification_sent isn't exercised by tests: current tests only append to listener.child_error_summaries manually. Add a test that feeds a non-scratch CellNotification with output.channel == MARIMO_ERROR through on_notification_sent and asserts the summary is captured (and propagates into build_done_event/extract_result).
| ): | ||
| err = msg.output.data[0] | ||
| exc_type = ( | ||
| getattr(err, "exception_type", None) or type(err).__name__ |
There was a problem hiding this comment.
This line is overlong and may fail formatting/linting checks (and hurts readability). Please wrap the exc_type = ... assignment across multiple lines (Black/Ruff-friendly).
| getattr(err, "exception_type", None) or type(err).__name__ | |
| getattr(err, "exception_type", None) | |
| or type(err).__name__ |
Truncating to 8 chars collapsed every cell in an embedded app to the same label, since embedded cells share a 36-char UUID prefix (`<uuid>Hbol`). The summary is a machine-readable SSE payload, not a terminal line, so there's no width budget to save.
|
🚀 Development release published. You may be able to view the changes at https://marimo.app?v=0.23.2-dev58 |
Fixes a regression from #8376 brought back HTML-wrapping for errors in code mode. Additionally, errors in child cells now also cause the done event to report
success: false, soexecute-code.shcallers get exit code 1. Finally, simplifies_print_summaryto just report(error)in the for cells since it appears in the traceback already.