fix(workbench): explain a terminal_run timeout instead of only reporting it - #622
Merged
Merged
Conversation
…ing it terminal_run's timeout said only that the done marker never appeared in the capture file. That single message covers three unrelated faults — the console never became usable, so the command's own progress is unknown; the file read back empty, so the command never started; or the file held output, so the command was still running when the budget expired — and the caller cannot act until they are told apart. test_clone_positron has failed this way three times in the daily Workbench suite with no progress on any of them, because every occurrence produced the same uninformative line. Track the readback outcomes the polling loop already observes and report the matching diagnosis, quoting the captured output tail when there is one.
Contributor
There was a problem hiding this comment.
Pull request overview
Improves the debuggability of Workbench terminal_run timeouts by preserving and reporting readback state (successful read count, last content seen, and last readback error) so callers can distinguish between “readback never worked”, “readback worked but no output”, and “command likely still running”.
Changes:
- Add
_timeout_diagnostics()and_TIMEOUT_CONTENT_CHARSto produce actionable timeout messages based on polling-loop observations. - Track readback successes, last content, and last readback error inside
terminal_run()and append diagnostics to timeout errors. - Add selftests covering the three diagnostic branches (partial output, empty file, and no successful readbacks).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/vip_tests/workbench/exec.py | Adds timeout diagnostic helper and readback bookkeeping to enrich terminal_run timeout errors. |
| selftests/test_workbench_exec.py | Adds unit tests validating the new timeout diagnostic messaging for three failure modes. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+949
to
+953
| return ( | ||
| f"The capture file read back empty after {readback_successes} successful " | ||
| "reads, so the command appears never to have started -- suspect the " | ||
| "terminal never received the typed command." | ||
| ) |
Comment on lines
1074
to
+1078
| try: | ||
| _open_file_in_vscode_editor(page, donefile, timeout=5_000) | ||
| marker_text = _read_vscode_editor_text(page, timeout=5_000) | ||
| _close_active_editor(page) | ||
| except Exception: | ||
| readback_successes += 1 |
Contributor
|
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
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.
What
terminal_run's timeout discarded every piece of evidence the polling loop had gathered:Why it matters
That one message is consistent with three unrelated faults, and the caller cannot act until they are told apart:
test_git_ops.py::test_clone_positronhas failed this way three times in the rstudio-pro daily Workbench suite (2026-07-22, 07-27, 08-13), each time emitting the identical uninformative line, and each time leaving no way to tell which of the three it was. That is why there has been no progress on it.Approach
Track the readback outcomes the loop already observes — successes, the last content seen, the last readback error — and report the matching diagnosis, quoting the captured output tail (capped at 400 chars) when there is one. No change to control flow, retry behaviour, or timeouts.
The third case now reads:
which would have settled the 120s-vs-genuinely-wedged question on the first occurrence.
Tests
Three cases, one per branch: partial output quoted, empty file reported as never-started, all-readbacks-failed reporting the readback error. Full selftest suite green — 1827 passed, 3 skipped.
ruff checkandruff formatclean.Note
Deliberately does not raise
_TIMEOUT_GIT_NETWORK. Whether 120s is too short is exactly what the current message makes unanswerable; this change is what lets the next occurrence answer it.