fix(workbench): raise ExecError immediately on terminal_run failure#445
fix(workbench): raise ExecError immediately on terminal_run failure#445ianpittwood wants to merge 3 commits into
Conversation
terminal_run only wrote its done-marker via `&&`, so a fast-failing command never wrote it, and the polling loop waited out the full timeout before raising a generic, misleading error with the real output discarded. The marker is now written unconditionally with the exit code appended, so failures surface immediately with the actual captured output. Fixes #439 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes terminal_run in the Workbench exec helpers so fast-failing terminal commands don’t get misreported as generic timeouts; instead, completion is detected via an unconditional done-marker that includes the command’s exit code, enabling immediate ExecError with the real captured output.
Changes:
- Write the terminal “done marker” unconditionally (
;instead of&&) and append:$?so failures still produce a completion marker. - Add
_parse_done_markerto parse(output, exit_code)and use it interminal_runto raise immediately on non-zero exit. - Add selftests for
_parse_done_markerandterminal_runbehaviors; add a Showboat demo validating the regression and test runs.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/vip_tests/workbench/exec.py |
Writes an unconditional marker with exit code, adds _parse_done_marker, and raises ExecError immediately on non-zero exit. |
selftests/test_workbench_exec.py |
Adds unit tests covering marker parsing and the updated terminal_run behavior (success, failure, timeout). |
validation_docs/demo-fix-terminal-run-timeout.md |
Adds a demo reproducing the old failure mode and showing the fix + test/lint runs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Preview Links
|
There was a problem hiding this comment.
The core fix is right and well covered. && becomes ;, $? is captured, a non-zero exit raises straight away, and the except ExecError: raise guard sits ahead of except Exception: pass in both poll paths. That last part is the easy one to get wrong, and it's correct here.
I traced every caller. Raising on a non-zero exit is safe everywhere: the clone/commit/push/deploy steps want it, file_exists runs an if/fi that always exits 0, git ls-remote exits 0, and the branch-delete cleanup already sits inside except ExecError. It even makes that cleanup faster.
Two things to fix before merge. Both live in _parse_done_marker, so the shell wrapper and its tests don't move. Suggestion is inline.
-
Glued marker (real, latent today). The wrapper appends
echo "{marker}:$?".echoadds a trailing newline but no leading one, so if a command's output doesn't end in\n, the marker sticks to the last line:fooVIP_DONE_abc:0. Theline.strip().startswith(prefix)check then never matches, the loop runs totimeout, and you get the generic "timed out" error. That is #439 again, this time on the success path. The old substring check tolerated it, so this is a regression. Nothing in the repo trips it now, since git, rsconnect, and theif/fiall print newline-terminated or empty output, butterminal_runis general DSL and a futureprintf fooorecho -ncaller would hang. -
Malformed exit code (minor).
int(...)on a non-numeric suffix, say a half-written marker line, throwsValueError.except Exception: passswallows it and the call times out instead of failing clean.$?is always numeric so this is close to impossible, but the reworked helper handles it for free by reading a not-yet-numeric suffix as still running.
Searching for the marker only at a line's start missed it when cmd's final output line had no trailing newline, since `>>` glues the marker onto that line with no separator -- causing terminal_run to poll until timeout again, the exact failure mode #439 fixed. Search for the marker anywhere in a line, keep any leading output on that line, and treat a non-digit exit-code suffix as still-running instead of raising. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The immediate-ExecError-on-failure logic is duplicated in both the RStudio/Positron (read_file) and VS Code (editor-open) polling loops, but only the former had test coverage -- a regression in the VS Code branch would have gone undetected. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
📸 Preview Screenshots8 screenshots captured across both preview deployments.
🌐 Website PreviewHomeURL: https://posit-dev.github.io/vip/pr-preview-site/pr-445/
Getting StartedURL: https://posit-dev.github.io/vip/pr-preview-site/pr-445/getting-started/
Test InventoryURL: https://posit-dev.github.io/vip/pr-preview-site/pr-445/tests/
Feature MatrixURL: https://posit-dev.github.io/vip/pr-preview-site/pr-445/feature-matrix/
Shiny AppURL: https://posit-dev.github.io/vip/pr-preview-site/pr-445/shiny-app/
Example Report (embedded)URL: https://posit-dev.github.io/vip/pr-preview-site/pr-445/report/
📊 Report PreviewSummary PageURL: https://posit-dev.github.io/vip/pr-preview/pr-445/
Detailed ResultsURL: https://posit-dev.github.io/vip/pr-preview/pr-445/details.html
Notes: The report pages ( Warning Firewall blocked 2 domainsThe following domains were blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "fonts.gstatic.com"
- "mtalk.google.com"See Network Configuration for more information.
|
📸 Preview ScreenshotsAutomatically captured screenshots for the preview deployments on this PR. 8 screenshots captured across 8 pages. 🌐 Website PreviewBase URL: https://posit-dev.github.io/vip/pr-preview-site/pr-445/ Home
Getting Started
Test Inventory
Feature Matrix
Example Report
Shiny App
📊 Report PreviewBase URL: https://posit-dev.github.io/vip/pr-preview/pr-445/ Summary (index)
Detailed Results
📋 URLs capturedWebsite pages (6):
Report pages (2):
Warning Firewall blocked 2 domainsThe following domains were blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "fonts.gstatic.com"
- "mtalk.google.com"See Network Configuration for more information.
|
Summary
terminal_run's shell wrapper only wrote its done-marker via&&, so a fast-failing command (non-zero exit) never wrote the marker at all -- the polling loop then waited out the full configured timeout and raised a generic, misleadingExecError, discarding the real output.;, with the command's exit code appended (marker:$?). A new pure helper,_parse_done_marker, parses it.terminal_runnow raisesExecErrorimmediately with the real captured output when the exit code is non-zero, instead of spinning until the timeout elapses. Commands that are genuinely still running (no marker at all) still time out as before.Fixes #439
Test plan
TestParseDoneMarker(6 cases) covering the new pure helperTestTerminalRun(4 cases, mocked Playwrightpage) covering: marker written unconditionally, success path, immediateExecErroron non-zero exit, and the still-hung timeout pathjust check(ruff lint + format) passesDemo
Fix #439: terminal_run swallows real command errors as a generic timeout
Root cause: terminal_run's shell wrapper only wrote the done-marker via
&&, so a fast-failing command (non-zero exit) never wrote the marker at all. The polling loop then waited out the full timeout and raised a generic 'timed out' error, discarding the real output sitting in the temp file the whole time.The fix writes the marker unconditionally via
;with the exit code appended (marker:$?), and a new pure helper_parse_done_markerparses it.terminal_runnow raisesExecErrorimmediately with the real captured output when the exit code is non-zero, instead of waiting out the full configured timeout.Before: the old
&&marker is silently dropped on failureThis reproduces the exact shell wrapper that used to run inside the IDE terminal.
After: the new
;marker is always written, with the exit codeThis reproduces the new shell wrapper used by the fixed
terminal_run.New unit tests covering the fix
_parse_done_markeris a pure, Playwright-free helper (matching this module's existing pattern) that parses the marker+exit-code line.terminal_runitself is also covered end-to-end with a mocked Playwrightpage, confirming: the marker is written unconditionally, success still returns output, a non-zero exit raisesExecErrorimmediately (single poll, not a full timeout loop), and a genuinely hung command still times out.Full selftest suite still passes
Lint and format checks pass
env -u VIRTUAL_ENV just check 2>&1🤖 Generated with Claude Code