fix(workbench): default git clone repo, accurate skips, and R readback#490
Merged
Conversation
Two related Workbench git-ops bugs surfaced validating a live 2026.06 deployment: - #479: push_* tests reported a misleading "Workbench session not established by --interactive-auth" skip when the real reason was a missing [workbench.git_test] block. The auth-session gate ran before the config gate, and flaky OIDC auth made the surfaced reason non-deterministic. Reorder the Gherkin steps so the config (and, for push, the read-only) gate runs before the login step, surfacing the real reason deterministically without depending on a login attempt. - #483: cloning a public repo wrongly appeared to require VIP_GIT_TOKEN. Ship a default public clone repo (posit-dev/posit-cli, auth_method "none") so the anonymous clone scenarios run out-of-the-box with no config, and reword the skip messages so public cloning is shown to need only clone_url + auth_method "none" (no token); a token is required only for push/private-repo scenarios. Fixes #479 Fixes #483
read_file()'s R branches evaluated a bare paste(readLines(...)) expression, which the R REPL auto-prints as a quoted, backslash-escaped character vector. That appended a stray '"' to the terminal_run done-marker line (...:0"), so _parse_done_marker rejected the exit code as non-numeric and terminal_run spun until its 120s timeout even though the command had finished in ~1.5s. Wrap the read expression in cat() (new _read_file_r_expr helper) so R emits raw bytes, and drop the now-needless _strip_r_index on those reads. Fixes the RStudio git-ops clone readback (surfaced end-to-end once #483 made clone run by default) and the identical Positron R-console readback path -- the readback half of #390. The Positron launch gate (#477) is tracked separately and needs a Positron-enabled deployment to validate. Refs #390 Refs #477
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a set of Workbench git-ops test reliability and usability issues by (1) making clone scenarios runnable out-of-the-box via a default public repo, (2) ensuring skip reasons are deterministic by gating config/read-only checks before login, and (3) fixing an R console readback bug that could prevent done-marker parsing and cause timeouts.
Changes:
- Synthesize a default
[workbench.git_test]config (public repo,auth_method="none") when the block is absent, and update skip messaging accordingly. - Reorder Gherkin steps so config/push gating runs before the (potentially flaky) interactive login step; add selftests guarding step order and skip text.
- Fix R readback by wrapping
readLines()output incat()via a shared helper, preventing R auto-print quoting from corrupting the done-marker exit code.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| vip.toml.example | Updates example docs to reflect optional git_test block + default anonymous clone behavior. |
| validation_docs/demo-issue-479-483-git-skip-gate.md | Adds Showboat demo documenting validation steps and outcomes. |
| src/vip/config.py | Adds default public clone URL constant and synthesizes default git_test config in WorkbenchConfig.from_dict. |
| src/vip_tests/workbench/test_git_ops.py | Updates module docs and improves skip messaging for public-clone vs token-required scenarios. |
| src/vip_tests/workbench/test_git_ops.feature | Reorders scenario steps so config/push gates run before login. |
| src/vip_tests/workbench/exec.py | Adds _read_file_r_expr() and routes R readback through cat() to avoid quoted done-marker corruption. |
| selftests/test_workbench_git_ops.py | Adds selftests for feature step-order guard and skip-message wording. |
| selftests/test_workbench_exec.py | Adds regression tests for quoted vs raw done-marker parsing and _read_file_r_expr behavior. |
| selftests/test_config.py | Updates expectations to confirm default git_test synthesis and token handling when block is absent. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
792
to
+795
| if ide == "vscode": | ||
| return read_file_via_vscode_editor(page, path, timeout=timeout) | ||
| # RStudio (and unknown — fall back to RStudio R path) | ||
| if lang.lower() == "r": | ||
| expr = f'paste(readLines("{path}"), collapse="\\n")' | ||
| return _strip_r_index(rstudio_eval(page, expr, timeout=timeout)) | ||
| expr = f'paste(readLines("{path}"), collapse="\\n")' | ||
| return _strip_r_index(rstudio_eval(page, expr, timeout=timeout)) | ||
| return rstudio_eval(page, _read_file_r_expr(path), timeout=timeout) |
Contributor
📸 Preview Screenshots7 screenshots captured from the PR preview deployments. Website Preview (
|
| Page | URL |
|---|---|
| Report Home | https://posit-dev.github.io/vip/pr-preview/pr-490/ |
| Detailed Results | https://posit-dev.github.io/vip/pr-preview/pr-490/details.html |
Notes: The report home full-page screenshot timed out (JS-heavy page); a viewport-only screenshot was captured instead. All other pages rendered successfully.
Warning
Firewall blocked 2 domains
The following domains were blocked by the firewall during workflow execution:
fonts.gstatic.commtalk.google.com
To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "fonts.gstatic.com"
- "mtalk.google.com"See Network Configuration for more information.
Generated by Capture preview screenshots for PRs · 56.6 AIC · ⌖ 6.42 AIC · ⊞ 6.1K · ◷
|
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.
Summary
Fixes a cluster of Workbench git-ops issues found validating a live
Workbench 2026.06 deployment. Three related fixes, each validated:
auth error when the real reason was missing git config. Reordered the
skip gates so the config/read-only gate resolves before the login step.
Ship a default public clone repo (posit-dev/posit-cli, auth_method
"none") so clone runs out-of-the-box, and reword the skip messages.
clone surfaced a real bug: read_file()'s R branches used a bare
paste(readLines(...)), which R auto-prints quoted/escaped, appending a
stray
"to the done-marker line so _parse_done_marker never acceptedthe exit code → 120s timeout. Wrap the read in cat() (new
_read_file_r_expr helper). Also fixes the Positron R-console readback
path (the readback half of fix(workbench): Positron git-ops readback times out (console proven in isolation) #390).
Live validation (dev.demo.posit.team, config-less, interactive auth)
test_clone_vscodePASSED — posit-cli default clones end-to-end, zero config (fix(workbench): public-repo cloning wrongly appears to require VIP_GIT_TOKEN #483)test_clone_rstudioPASSED in ~23s — was a 120s timeout before the readback fixtest_push_*SKIPPED with the accurate read-only reason, before any login (fix(workbench): git-ops push_* tests report misleading 'session not established' skip when git_test config is absent #479)Not in this PR
.positron-consolelaunch-gate false negative) isdeferred — it needs a Positron-enabled deployment to validate (dev.demo
has no Positron; the reproducing deployment is VPN-gated).
Test plan
step-order guard, skip-message wording, readback regression tests)
Demo
Fix: workbench git-ops skip reasons + default public clone repo
This branch fixes two related Workbench git-ops bugs.
#479 — misleading auth skip when git config is absent. Every scenario in test_git_ops.feature checked the auth-session login gate BEFORE the git-config gate. Because Workbench OIDC auth is flaky under the shared-account serial run, whether a git test skipped with the config reason or the auth reason was non-deterministic — push tests could report a misleading 'Workbench session not established' error when the real reason was 'no git config / read-only'. Fixed by reordering the Gherkin steps so the config gate (and, for push scenarios, the pushing gate) resolves before the login attempt.
#483 — public-repo cloning wrongly appeared to require VIP_GIT_TOKEN. There was no default [workbench.git_test] config, so clone scenarios always skipped without one, and the skip message read as if a token was required even for anonymous public clones. Fixed by shipping a default public clone target (
https://github.com/posit-dev/posit-cli.git, auth_method="none") that WorkbenchConfig.from_dict synthesizes when the block is absent, and by rewording the skip messages to make clear a token is only needed for push/private-repo scenarios.Fix #479: config gate now precedes login in every scenario
Before this fix, step 1 was the login attempt and step 2 was the config check. Now the config check runs first:
grep -n -A4 "Scenario: Clone a Git repository in RStudio" src/vip_tests/workbench/test_git_ops.featureFix #483: reworded skip message makes clear public clones need no token
The config-missing skip message now explicitly says an anonymous public-repo clone needs only
clone_url+auth_method='none'(no token), and a token is only required for push/private-repo scenarios:sed -n "219,225p" src/vip_tests/workbench/test_git_ops.pyNew selftests pass: default-config synthesis, step-order guard, skip-message wording
Full selftest suite is green (excluding
selftests/test_load_engine.py, which has pre-existing timing-flaky tests unrelated to this change; run with-n0since the repo's default-n autoxdist parallelism is itself intermittently flaky, unrelated to this change)Lint and format check (
just checkequivalent — run directly with the project-scoped uv wrapper sincejust's recipes invoke bareuv, which reroots to the parent ptd-workspace in this environment)env -u UV_PROJECT uv run --frozen --no-sync --project . ruff check src/ src/vip_tests/ selftests/ examples/env -u UV_PROJECT uv run --frozen --no-sync --project . ruff format --check src/ src/vip_tests/ selftests/ examples/The 6 git-ops scenarios still collect after the feature-file reorder
Workbench product tests deselect entirely without a configured URL, so this confirms collection with a minimal
[workbench]config:Follow-up fix: R file-readback quoting bug (surfaced once #483 made clone run by default)
Live validation of #483's default clone against dev.demo.posit.team exposed a third bug:
read_file()'s R branches evaluated a barepaste(readLines(...), collapse="\\n")expression. The R REPL auto-prints that as a quoted, backslash-escaped character vector, which appended a stray"to theterminal_rundone-marker line (...:0\")._parse_done_markerthen rejected the exit code as non-numeric, soterminal_runspun until its 120s timeout even though the underlying command had finished in ~1.5s — this is what madetest_clone_rstudiotime out.The fix wraps the read in
cat()via a new_read_file_r_exprhelper, so R emits raw bytes instead of an auto-printed quoted vector, and drops the now-unnecessary_strip_r_indexcall on those reads (both the RStudio and Positron R-console readback paths used the same buggy expression).grep -n "cat(paste(readLines" src/vip_tests/workbench/exec.pyLive validation on dev.demo.posit.team
Not re-runnable in this document (requires a live deployment + interactive browser auth), so recorded here as prose instead of an exec block:
Running a config-less
--interactive-authpass against dev.demo.posit.team (a real 2026.06-class deployment):test_clone_vscodePASSED — proves the fix(workbench): public-repo cloning wrongly appears to require VIP_GIT_TOKEN #483 default clone (posit-dev/posit-cli, no config, no token) works end-to-end against a real Workbench instance.test_clone_rstudioPASSED in ~23s — before this readback fix it was hitting the full 120sterminal_runtimeout; thecat()wrap resolved the done-marker parsing failure described above.auth_method='none' is anonymous (read-only)...), confirming the fix(workbench): git-ops push_* tests report misleading 'session not established' skip when git_test config is absent #479 reorder surfaces the real skip reason instead of a login-flake error.test_clone_positron/test_push_positronwere not exercised — dev.demo has no Positron IDE installed, so the Positron launch gate (fix(workbench): test_launch_positron skips (.positron-console not found) though Positron is installed #477) remains deferred pending a Positron-enabled deployment.