fix(workbench): default git clone repo, accurate skips, and R readback (#490)

Author: ian-flores

selftests/test_config.py

@@ -5,6 +5,7 @@
 import pytest
 
 from vip.config import (
+    DEFAULT_PUBLIC_CLONE_URL,
     AuthConfig,
     ConnectConfig,
     GitTestConfig,
@@ -670,9 +671,12 @@ def test_git_test_absent_by_default(self):
         wc = WorkbenchConfig(url="https://workbench.example.com")
         assert wc.git_test is None
 
-    def test_git_test_absent_from_dict_when_key_missing(self):
+    def test_git_test_absent_from_dict_yields_default_public_repo(self):
         wc = WorkbenchConfig.from_dict({"url": "https://workbench.example.com"})
-        assert wc.git_test is None
+        assert wc.git_test is not None
+        assert wc.git_test.clone_url == DEFAULT_PUBLIC_CLONE_URL
+        assert wc.git_test.auth_method == "none"
+        assert wc.git_test.token == ""
 
     def test_git_test_parsed_from_dict(self):
         wc = WorkbenchConfig.from_dict(
@@ -695,10 +699,12 @@ def test_git_test_token_from_env(self, monkeypatch):
         assert wc.git_test is not None
         assert wc.git_test.token == "tok123"
 
-    def test_git_test_none_when_env_not_set_and_no_block(self, monkeypatch):
-        monkeypatch.delenv("VIP_GIT_TOKEN", raising=False)
+    def test_git_test_default_ignores_env_token(self, monkeypatch):
+        """auth_method='none' forces the default's token empty even with VIP_GIT_TOKEN set."""
+        monkeypatch.setenv("VIP_GIT_TOKEN", "tok123")
         wc = WorkbenchConfig.from_dict({"url": "https://workbench.example.com"})
-        assert wc.git_test is None
+        assert wc.git_test is not None
+        assert wc.git_test.token == ""
 
 
 class TestLoadConfigGitTest:
@@ -719,15 +725,18 @@ def test_git_test_block_parsed(self, tmp_toml, monkeypatch):
         assert cfg.workbench.git_test.clone_url == "https://github.com/org/repo.git"
         assert cfg.workbench.git_test.auth_method == "https-token"
 
-    def test_git_test_block_absent_yields_none(self, tmp_toml):
+    def test_git_test_block_absent_yields_default_public_repo(self, tmp_toml):
         path = tmp_toml(
             """
 [workbench]
 url = "https://workbench.example.com"
 """
         )
         cfg = load_config(path)
-        assert cfg.workbench.git_test is None
+        assert cfg.workbench.git_test is not None
+        assert cfg.workbench.git_test.clone_url == DEFAULT_PUBLIC_CLONE_URL
+        assert cfg.workbench.git_test.auth_method == "none"
+        assert cfg.workbench.git_test.token == ""
 
     def test_git_test_token_from_env(self, tmp_toml, monkeypatch):
         monkeypatch.setenv("VIP_GIT_TOKEN", "env-token-xyz")

selftests/test_workbench_exec.py

@@ -28,6 +28,7 @@
     _extract_between_markers,
     _make_sentinels,
     _parse_done_marker,
+    _read_file_r_expr,
     _split_marker,
     _strip_r_index,
     _wrap_python_expr,
@@ -345,6 +346,39 @@ def test_returns_none_for_non_digit_suffix(self):
         content = "VIP_DONE_abc:"
         assert _parse_done_marker(content, "VIP_DONE_abc") is None
 
+    def test_raw_exit_code_parses_but_quoted_does_not(self):
+        """Regression guard: R's auto-print wraps a bare (un-cat'd) character
+        result in a trailing closing quote, turning ``...:0`` into ``...:0"``.
+        The raw form must parse as exit 0; the quoted form must be treated as
+        still-running rather than silently misparsed. This is why
+        ``_read_file_r_expr`` wraps its read expression in ``cat()`` -- so the
+        marker line R's console echoes back is always the raw form."""
+        raw = "hello\nVIP_DONE_abc:0"
+        quoted = 'hello\nVIP_DONE_abc:0"'
+        assert _parse_done_marker(raw, "VIP_DONE_abc") == ("hello", 0)
+        assert _parse_done_marker(quoted, "VIP_DONE_abc") is None
+
+
+# ---------------------------------------------------------------------------
+# _read_file_r_expr
+# ---------------------------------------------------------------------------
+
+
+class TestReadFileRExpr:
+    def test_wraps_read_in_cat(self):
+        """The read expression must be cat()'d, not left as a bare/auto-printed
+        expression -- R's console auto-print wraps a bare character result in
+        quotes and escapes embedded newlines as literal ``\\n``, which broke
+        the done-marker exit-code parse (see TestParseDoneMarker above)."""
+        expr = _read_file_r_expr("/tmp/foo.txt")
+        assert expr.startswith("cat(")
+        assert "readLines(" in expr
+        assert "paste(" in expr
+
+    def test_embeds_path(self):
+        expr = _read_file_r_expr("/tmp/vip_term_abc.txt")
+        assert '"/tmp/vip_term_abc.txt"' in expr
+
 
 # ---------------------------------------------------------------------------
 # _detect_ide
@@ -497,7 +531,9 @@ class TestReadFileRouting:
     def test_rstudio_calls_rstudio_eval(self, monkeypatch):
         page = MagicMock()
         monkeypatch.setattr(exec_mod, "_detect_ide", lambda p: "rstudio")
-        mock_rstudio_eval = MagicMock(return_value="[1] hello world")
+        # cat()'d output is raw -- no "[1]" index, no quoting -- so the mock
+        # return value here is the *raw* form, not R's auto-printed form.
+        mock_rstudio_eval = MagicMock(return_value="hello world")
         mock_positron_eval_r = MagicMock()
         mock_positron_eval_python = MagicMock()
         mock_editor_read = MagicMock()
@@ -508,7 +544,9 @@ def test_rstudio_calls_rstudio_eval(self, monkeypatch):
 
         result = read_file(page, "/tmp/foo.txt", lang="r")
 
-        assert result == "hello world"  # _strip_r_index applied
+        assert result == "hello world"
+        rstudio_expr = mock_rstudio_eval.call_args[0][1]
+        assert rstudio_expr.startswith("cat(")  # not a bare auto-printed expr
         mock_rstudio_eval.assert_called_once()
         mock_positron_eval_r.assert_not_called()
         mock_positron_eval_python.assert_not_called()
@@ -518,7 +556,8 @@ def test_positron_r_calls_positron_eval_r(self, monkeypatch):
         page = MagicMock()
         monkeypatch.setattr(exec_mod, "_detect_ide", lambda p: "positron")
         mock_rstudio_eval = MagicMock()
-        mock_positron_eval_r = MagicMock(return_value="[1] file contents")
+        # cat()'d output is raw -- no "[1]" index, no quoting.
+        mock_positron_eval_r = MagicMock(return_value="file contents")
         mock_positron_eval_python = MagicMock()
         mock_editor_read = MagicMock()
         monkeypatch.setattr(exec_mod, "rstudio_eval", mock_rstudio_eval)
@@ -528,7 +567,9 @@ def test_positron_r_calls_positron_eval_r(self, monkeypatch):
 
         result = read_file(page, "/tmp/foo.txt", lang="r")
 
-        assert result == "file contents"  # _strip_r_index applied
+        assert result == "file contents"
+        positron_expr = mock_positron_eval_r.call_args[0][1]
+        assert positron_expr.startswith("cat(")  # not a bare auto-printed expr
         mock_positron_eval_r.assert_called_once()
         mock_rstudio_eval.assert_not_called()
         mock_positron_eval_python.assert_not_called()

selftests/test_workbench_git_ops.py

@@ -1,15 +1,28 @@
-"""Selftests for the Workbench git-ops clone command construction.
+"""Selftests for the Workbench git-ops clone command construction and skip gates.
 
 Covers ``_do_clone`` in ``vip_tests.workbench.test_git_ops``: verifies the
 shell command it builds removes any pre-existing clone directory before
 running ``git clone``, so repeat runs against long-lived QA VMs don't fail
 with "destination path already exists". No live Workbench deployment or
 Playwright browser required -- ``terminal_run`` is monkeypatched to capture
 the command string instead of executing it.
+
+Also covers the config/auth skip-gate wording (#483) and the Gherkin step
+order guard (#479): every scenario in test_git_ops.feature must resolve the
+Git config gate before attempting the (possibly flaky) login step, so a
+read-only/config situation is reported deterministically instead of being
+masked by a login bounce.
 """
 
 from __future__ import annotations
 
+from importlib.util import find_spec
+from pathlib import Path
+
+import pytest
+
+from vip.config import GitTestConfig, VIPConfig, WorkbenchConfig
+from vip.gherkin import parse_feature_file
 from vip_tests.workbench import test_git_ops as git_ops
 
 
@@ -57,3 +70,115 @@ def fake_terminal_run(page, cmd, timeout=30_000, *, readback_lang="r"):
 
     assert clone_dir == "/home/vip/other-repo"
     assert "rm -rf /home/vip/other-repo" in captured["cmd"]
+
+
+# ---------------------------------------------------------------------------
+# #479 -- Gherkin step order guard
+# ---------------------------------------------------------------------------
+
+
+def _git_ops_feature_path() -> Path:
+    spec = find_spec("vip_tests")
+    assert spec and spec.submodule_search_locations
+    return Path(spec.submodule_search_locations[0]) / "workbench" / "test_git_ops.feature"
+
+
+class TestFeatureStepOrder:
+    """The Git config gate must resolve before the (possibly flaky) login step.
+
+    Otherwise a read-only/config situation is masked by a non-deterministic
+    login bounce, surfacing a misleading auth error instead of the real
+    config/read-only reason (#479).
+    """
+
+    def test_config_gate_precedes_login_in_every_scenario(self):
+        parsed = parse_feature_file(_git_ops_feature_path())
+        assert parsed["scenarios"], "expected scenarios in test_git_ops.feature"
+        for scenario in parsed["scenarios"]:
+            steps = scenario["steps"]
+            config_idx = next(
+                i for i, s in enumerate(steps) if "the Git test config is available" in s
+            )
+            login_idx = next(
+                i for i, s in enumerate(steps) if "Workbench is accessible and I am logged in" in s
+            )
+            assert config_idx < login_idx, (
+                f"scenario {scenario['title']!r}: 'the Git test config is available' "
+                f"must precede 'Workbench is accessible and I am logged in', "
+                f"got steps: {steps}"
+            )
+
+    def test_push_scenarios_check_pushing_before_login(self):
+        parsed = parse_feature_file(_git_ops_feature_path())
+        push_scenarios = [s for s in parsed["scenarios"] if "push" in s["title"].lower()]
+        assert push_scenarios, "expected at least one push scenario"
+        for scenario in push_scenarios:
+            steps = scenario["steps"]
+            pushing_idx = next(
+                i for i, s in enumerate(steps) if "the Git test config supports pushing" in s
+            )
+            login_idx = next(
+                i for i, s in enumerate(steps) if "Workbench is accessible and I am logged in" in s
+            )
+            assert pushing_idx < login_idx, (
+                f"scenario {scenario['title']!r}: 'the Git test config supports pushing' "
+                f"must precede 'Workbench is accessible and I am logged in', "
+                f"got steps: {steps}"
+            )
+
+
+# ---------------------------------------------------------------------------
+# #483 -- skip-message wording
+# ---------------------------------------------------------------------------
+
+
+def _make_vip_config(git_test: GitTestConfig | None) -> VIPConfig:
+    wc = WorkbenchConfig(url="https://workbench.example.com", git_test=git_test)
+    return VIPConfig(workbench=wc)
+
+
+class TestGitConfigAvailableSkipMessages:
+    def test_none_config_message_explains_public_vs_token(self):
+        """Defensive fallback: git_test is None only via direct construction."""
+        cfg = _make_vip_config(None)
+        with pytest.raises(pytest.skip.Exception) as exc_info:
+            git_ops.git_config_available(cfg)
+        msg = exc_info.value.msg
+        assert "auth_method='none'" in msg
+        assert "no token required" in msg
+        assert "VIP_GIT_TOKEN" in msg
+
+    def test_empty_clone_url_message_explains_public_vs_token(self):
+        cfg = _make_vip_config(GitTestConfig(clone_url="", auth_method="none"))
+        with pytest.raises(pytest.skip.Exception) as exc_info:
+            git_ops.git_config_available(cfg)
+        msg = exc_info.value.msg
+        assert "auth_method='none'" in msg
+        assert "no token" in msg
+
+    def test_valid_none_config_does_not_skip(self):
+        cfg = _make_vip_config(
+            GitTestConfig(
+                clone_url="https://github.com/posit-dev/posit-cli.git", auth_method="none"
+            )
+        )
+        result = git_ops.git_config_available(cfg)
+        assert result is cfg.workbench.git_test
+
+
+class TestGitConfigSupportsPushingSkipMessage:
+    def test_anonymous_auth_skip_message(self):
+        git_cfg = GitTestConfig(clone_url="https://github.com/org/repo.git", auth_method="none")
+        with pytest.raises(pytest.skip.Exception) as exc_info:
+            git_ops.git_config_supports_pushing(git_cfg)
+        msg = exc_info.value.msg
+        assert "auth_method='none' is anonymous (read-only)" in msg
+        assert "auth_method='https-token'" in msg
+        assert "VIP_GIT_TOKEN" in msg
+
+    def test_https_token_auth_does_not_skip(self):
+        git_cfg = GitTestConfig(
+            clone_url="https://github.com/org/repo.git", auth_method="https-token", token="tok"
+        )
+        # Should not raise/skip.
+        git_ops.git_config_supports_pushing(git_cfg)

src/vip/config.py

@@ -168,13 +168,21 @@ def from_dict(cls, raw: dict) -> WorkbenchExtensionsConfig:
         )
 
 
+# Default anonymous-clone target used when [workbench.git_test] is absent, so
+# the clone/connectivity scenarios have something to run against out-of-the-box.
+DEFAULT_PUBLIC_CLONE_URL = "https://github.com/posit-dev/posit-cli.git"
+
+
 @dataclass
 class GitTestConfig:
     """Configuration for [workbench.git_test] Git operations testing.
 
     Enables terminal and GUI Git scenarios (clone, branch, commit, push)
-    against a real Git repository.  All scenarios auto-skip when this block
-    is absent from the config file.
+    against a real Git repository.  When this block is absent from the config
+    file, ``WorkbenchConfig.from_dict`` synthesizes a default pointing at a
+    public repository (``DEFAULT_PUBLIC_CLONE_URL``) with ``auth_method =
+    "none"``, so clone/connectivity scenarios run out-of-the-box; push/commit
+    scenarios skip as read-only until the user configures a token.
 
     Token is never stored in the TOML file.  For ``auth_method = "https-token"``
     set VIP_GIT_TOKEN in the environment (same pattern as
@@ -242,6 +250,9 @@ class WorkbenchConfig(ProductConfig):
     idle_grace_seconds: int = 60
     extensions: WorkbenchExtensionsConfig = field(default_factory=WorkbenchExtensionsConfig)
     kubernetes: WorkbenchKubernetesConfig = field(default_factory=WorkbenchKubernetesConfig)
+    # None only when constructed directly (e.g. in tests); ``from_dict`` always
+    # populates this, defaulting to an anonymous public-repo clone when
+    # [workbench.git_test] is absent from the TOML file.
     git_test: GitTestConfig | None = None
     # Base path of Chronicle's on-disk data on the Workbench server.  Defaults
     # to the embedded-Workbench location under the shared-storage tree.  The
@@ -286,7 +297,11 @@ def from_dict(cls, raw: dict) -> WorkbenchConfig:
             idle_grace_seconds=raw.get("idle_grace_seconds", 60),
             extensions=WorkbenchExtensionsConfig.from_dict(raw.get("extensions", {})),
             kubernetes=WorkbenchKubernetesConfig.from_dict(raw.get("kubernetes", {})),
-            git_test=GitTestConfig.from_dict(git_test_raw) if git_test_raw is not None else None,
+            git_test=(
+                GitTestConfig.from_dict(git_test_raw)
+                if git_test_raw is not None
+                else GitTestConfig(clone_url=DEFAULT_PUBLIC_CLONE_URL, auth_method="none")
+            ),
             chronicle_data_path=raw.get(
                 "chronicle_data_path", "/var/lib/rstudio-server/shared-storage/chronicle"
             ),

src/vip_tests/workbench/exec.py

@@ -85,6 +85,18 @@ def _wrap_r_expr(expr: str, start: str, end: str) -> str:
     return f'cat("{s1}", "{s2}\\n", sep=""); {expr}; cat("\\n", "{e1}", "{e2}\\n", sep="")'
 
 
+def _read_file_r_expr(path: str) -> str:
+    """Build the R expression that reads *path* and emits its raw contents.
+
+    Wrapped in ``cat()`` so R prints the file bytes directly. A bare
+    ``paste(readLines(...))`` is auto-printed by the R REPL as a quoted,
+    backslash-escaped character vector -- which appends a stray ``"`` to the
+    done-marker line (``...:0"``) and makes ``_parse_done_marker`` reject the
+    exit code as non-numeric, hanging ``terminal_run`` until timeout.
+    """
+    return f'cat(paste(readLines("{path}"), collapse="\\n"))'
+
+
 def _wrap_python_expr(expr: str, start: str, end: str) -> str:
     """Wrap *expr* with Python print() markers.
 
@@ -773,16 +785,11 @@ def read_file(page: Page, path: str, timeout: int = 30_000, *, lang: str = "r")
     ide = _detect_ide(page)
     if ide == "positron":
         if lang.lower() == "r":
-            expr = f'paste(readLines("{path}"), collapse="\\n")'
-            return _strip_r_index(positron_eval_r(page, expr, timeout=timeout))
+            return positron_eval_r(page, _read_file_r_expr(path), timeout=timeout)
         return positron_eval_python(
             page, f'with open("{path}") as _f: print(_f.read())', timeout=timeout
         )
     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)